Add ADC Dual Mode support with new AdvancedADCDual class #64
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Revised change based on feedback. Added new class AdvancedADCDual. Modified AdvancedADC.begin() to accept optional start and fixed ADC number to allow Dual Mode to be called from AdvancedADCDual.
Added enable_dual_mode and disable_dual_mode to support Dual Mode ADC
Added enable_dual_mode and disable_dual_mode declarations
|
Memory usage change @ 73b2ac6
Click for full report table
Click for full report CSV |
|
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: #60 (comment) Also, can you please format your code like the rest of the library ? I'll leave a few hints in a review. |
iabdalkader
requested changes
Mar 19, 2024
src/AdvancedADC.cpp
Outdated
| @@ -115,42 +115,73 @@ DMABuffer<Sample> &AdvancedADC::read() { | |||
| return NULLBUF; | |||
| } | |||
|
|
|||
| int AdvancedADC::begin(uint32_t resolution, uint32_t sample_rate, size_t n_samples, size_t n_buffers) { | |||
| int AdvancedADC::begin(uint32_t resolution, uint32_t sample_rate, size_t n_samples, size_t n_buffers,bool do_start,uint8_t adcNum) { | |||
There was a problem hiding this comment.
Suggested change
| int AdvancedADC::begin(uint32_t resolution, uint32_t sample_rate, size_t n_samples, size_t n_buffers,bool do_start,uint8_t adcNum) { | |
| int AdvancedADC::begin(uint32_t resolution, uint32_t sample_rate, size_t n_samples, size_t n_buffers,bool do_start, uint8_t adc_num) { |
Space between args, and No camel case please.
src/AdvancedADC.cpp
Outdated
Comment on lines
127
to
131
| if(adcNum>AN_ARRAY_SIZE(adc_pin_alt)) | ||
| { | ||
| descr = nullptr; | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Suggested change
| if(adcNum>AN_ARRAY_SIZE(adc_pin_alt)) | |
| { | |
| descr = nullptr; | |
| return 0; | |
| } | |
| if (adcNum > AN_ARRAY_SIZE(adc_pin_alt)) { | |
| descr = nullptr; | |
| return 0; | |
| } |
Spaces between operators, bracket on the same line as function/if.
src/AdvancedADC.cpp
Outdated
| } | ||
| } | ||
| } | ||
| } | ||
| else if(adcNum>0) //if ADC specified use that ADC to try to map first channel |
There was a problem hiding this comment.
Suggested change
| else if(adcNum>0) //if ADC specified use that ADC to try to map first channel | |
| } else if (adcNum > 0) { //if ADC specified use that ADC to try to map first channel |
src/AdvancedADC.cpp
Outdated
| adc_pins[0] = pin; | ||
| } | ||
| } | ||
| selectedADC=adcNum; //Store selected number |
There was a problem hiding this comment.
Suggested change
| selectedADC=adcNum; //Store selected number | |
| selected_adc = adc_num; //Store selected number |
src/AdvancedADC.cpp
Outdated
Comment on lines
276
to
280
| if(do_start) | ||
| { | ||
| return(start(sample_rate)); | ||
| } | ||
|
|
There was a problem hiding this comment.
Suggested change
| if(do_start) | |
| { | |
| return(start(sample_rate)); | |
| } | |
| if (do_start) { | |
| return start(sample_rate); | |
| } |
No return() please.
| @@ -206,7 +218,7 @@ int hal_adc_config(ADC_HandleTypeDef *adc, uint32_t resolution, uint32_t trigger | |||
| sConfig.OffsetNumber = ADC_OFFSET_NONE; | |||
| sConfig.SingleDiff = ADC_SINGLE_ENDED; | |||
| sConfig.SamplingTime = ADC_SAMPLETIME_8CYCLES_5; | |||
|
|
|||
| @@ -46,18 +46,18 @@ int hal_tim_config(TIM_HandleTypeDef *tim, uint32_t t_freq) { | |||
| sConfig.MasterSlaveMode = TIM_MASTERSLAVEMODE_ENABLE; | |||
|
|
|||
| if (tim->Instance == TIM1) { | |||
| __HAL_RCC_TIM1_CLK_ENABLE(); | |||
| __HAL_RCC_TIM1_CLK_ENABLE(); | |||
There was a problem hiding this comment.
Suggested change
| __HAL_RCC_TIM1_CLK_ENABLE(); | |
| __HAL_RCC_TIM1_CLK_ENABLE(); |
Please remove any spaces you added.
Comment on lines
+67
to
+71
|
|
||
| //void setADCDualMode(bool dm); | ||
| //int enableDualMode(); | ||
| //int disableDualMode(); | ||
|
|
There was a problem hiding this comment.
Suggested change
| //void setADCDualMode(bool dm); | |
| //int enableDualMode(); | |
| //int disableDualMode(); | |
Comment on lines
+79
to
+80
| AdvancedADC *adcIN1; | ||
| AdvancedADC *adcIN2; |
There was a problem hiding this comment.
Suggested change
| AdvancedADC *adcIN1; | |
| AdvancedADC *adcIN2; | |
| AdvancedADC &adc1; | |
| AdvancedADC &adc2; |
Those should be passed by reference.
src/AdvancedADC.h
Outdated
| } | ||
| AdvancedADC(): n_channels(0), descr(nullptr) {} | ||
| ~AdvancedADC(); | ||
| bool available(); | ||
| SampleBuffer read(); | ||
| int begin(uint32_t resolution, uint32_t sample_rate, size_t n_samples, size_t n_buffers); | ||
| int begin(uint32_t resolution, uint32_t sample_rate, size_t n_samples, size_t n_buffers, size_t n_pins, pin_size_t *pins) { | ||
| int begin(uint32_t resolution, uint32_t sample_rate, size_t n_samples, size_t n_buffers, bool do_start=true,uint8_t adcNum=0); |
There was a problem hiding this comment.
Suggested change
| int begin(uint32_t resolution, uint32_t sample_rate, size_t n_samples, size_t n_buffers, bool do_start=true,uint8_t adcNum=0); | |
| int begin(uint32_t resolution, uint32_t sample_rate, size_t n_samples, size_t n_buffers, bool start=true, uint8_t adc_instance=0); |
|
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. |
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
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. |
Fixed formatting
Fixed formatting
|
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 @ 1d11f4f
Click for full report table
Click for full report CSV |
per1234
added
type: enhancement
More formatting fixes and removed need for adc_num.
|
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. 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 @ 4f467e2
Click for full report table
Click for full report CSV |
Removed a couple of debug statements I left hanging around
|
Memory usage change @ e3d00aa
Click for full report table
Click for full report CSV |
|
I'll refactor this PR, rebase it and push it again. |
per1234
reviewed
Mar 24, 2024
There was a problem hiding this comment.
Closing as superseded by #65
Thanks for your contribution @jmdodd95682!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The proposed change adds a new class
AdvancedADCDualclass 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
pinsis 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_startandadcNum.Additionally, a new
start()routine was added toAdvancedADC().The
do_startparameter allows the AdvancedADCDual to hold-off starting the ADC until both are configured.The
adcNumis required because you must force the input pins to be mapped to ADC1 and ADC2 for dual mode.Since both parameters are defaulted, the
AdvancedADC.begin()remains backward compatible with existing code.Finally, the
HALConfig.cppand.hwere modified to add two new routines:hal_enable_dual_modeandhal_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.