Closed jmdodd95682 closed 6 months ago
Memory usage change @ 73b2ac64a8141a7fbd23bcee25933660bd35e41d
Board | flash | % | RAM for global variables | % |
---|---|---|---|---|
arduino:mbed_giga:giga |
:small_red_triangle: 0 - +256 | 0.0 - +0.01 | 0 - 0 | 0.0 - 0.0 |
Hi! Thanks for sending this! I'll test it tomorrow. In the meantime, do we need to specify the ADC number ? See my comment here: https://github.com/arduino-libraries/Arduino_AdvancedAnalog/pull/60#issuecomment-2004152259
Also, can you please format your code like the rest of the library ? I'll leave a few hints in a review.
I could not locate your specific comment on the ADC number in #60. Probably me.
Anyway, the reason we need the ADC number is that Dual mode only works if you can use ADC1 and ADC2. It does NOT work with ADC3. Nor can you combine ADC1 and 3...etc.
So, if someone has already declared an instance of AdvancedADC and executed a begin() then probably ADC1 will be taken. And then ADC dual will not work. The existing begin() will just try to use ADC2 and ADC3, and programming the Dual field will make ADC1 no longer work correctly. And ADC3 will never start and won't be synchronized.
In addition, the start MUST be done to ADC1 (not ADC2), so the order in which you declare them matters somewhat if your application wants to start capturing based on a triggering condition (e.g. rising edge) of whatever isconnected to ADC1.
So, I THINK you need the adc number specified. for each begin() to make Dual mode work reliably.
So, if someone has already declared an instance of AdvancedADC and executed
This would be a user error. We can check if the instances are correct, and check which one is ADC1 and call start on that. And we'll add a comment about that in the example.
Here's my comment from #60
Note I don't think we need to specify the ADC number, which should make things easier. The first pin always determines the ADC instance, and the rest of the pins must map to the same ADC (this code is already implemented). The instances/descriptors are also allocated in order, so you'll get ADC1 first, then ADC2, then ADC3. The AdvancedADCDual can also check if they are the correct ADCs or not.
EDIT @jmdodd95682 btw following the same logic, someone can still pass ADC 2 and 3, or 1 and 3, we can't stop users from doing that but we can detect the error.
Fair enough. Yes, you can do it this way. It does end up in the same place, without the need for another parameter being passed to begin(). Either way if you ask to ADC1 and ADC2 and cannot get it, it just gives you an error. Good point.
I can make this change.
Memory usage change @ 1d11f4fd01266d99269acf3da26acd44b3d1916b
Board | flash | % | RAM for global variables | % |
---|---|---|---|---|
arduino:mbed_giga:giga |
:small_red_triangle: 0 - +256 | 0.0 - +0.01 | 0 - 0 | 0.0 - 0.0 |
Okay. I removed the adc_num from the AdvancedADC.begin().
The AdvancedADCDual.begin() now checks that the assigned ADC for the first pin is ADC1 and all subsequent pins is ADC2. The changes to AdvancedADC.begin() are now much simpler.
Also added a new member function in AdvancedADC.getAssignedADC() so that AdvancedADCDual can determine which ADC was assigned and flag an error if its not correct.
I tested it, it works reliably on my app.
Memory usage change @ 4f467e252a1670f54ee337d46670b150c1c69262
Board | flash | % | RAM for global variables | % |
---|---|---|---|---|
arduino:mbed_giga:giga |
:small_red_triangle: 0 - +64 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
Memory usage change @ e3d00aaa667a896fcc6043a1f8d9aaa8c44b2ad7
Board | flash | % | RAM for global variables | % |
---|---|---|---|---|
arduino:mbed_giga:giga |
:small_red_triangle: 0 - +64 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
I'll refactor this PR, rebase it and push it again.
The proposed change adds a new class
AdvancedADCDual
class which is used to handle configuring ADC1 and ADC2 and starting them synchronously by enabling Dual mode in the ADC1/2_CCR register.It works like this:
where
pins
is an array of pins numbers.The AdvancedADCDual
begin()
routine calls the AdvancedADCbegin()
for each instance.It configures the first pin in the array on ADC1 and the remaining pins on ADC2. There was not a cleaner way to do this without passing two arrays (which is possible). Finally it calls
start()
on the AdvancedADC mapped to ADC1.The
AdvancedADC.begin()
routine was modified to take two optional parameters:do_start
andadcNum
. Additionally, a newstart()
routine was added toAdvancedADC()
. Thedo_start
parameter allows the AdvancedADCDual to hold-off starting the ADC until both are configured. TheadcNum
is required because you must force the input pins to be mapped to ADC1 and ADC2 for dual mode. Since both parameters are defaulted, theAdvancedADC.begin()
remains backward compatible with existing code.Finally, the
HALConfig.cpp
and.h
were modified to add two new routines:hal_enable_dual_mode
andhal_disable_dual_mode
. These access the low-level driver to write a 5-bit value to the ADC1/2_CCR register enabling or disabling Dual Mode Simultaneous.I've tested this, and it seems to work properly.