-
Notifications
You must be signed in to change notification settings - Fork 4
Add support for dual ADC mode. #65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@jmdodd95682 I rebased the PR and cleaned it up here, can you please give this a try ? There's an included example, but if you have a better one please send it. Was there any reason we needed to limit the number of channels per ADC to 1 in dual mode ? |
a4dbc1a
to
0b6f3fb
Compare
Memory usage change @ 0b6f3fb
Click for full report table
Click for full report CSV
|
I limited ADC1 to a single input pin, and the remaining pins are assigned to ADC2. The reason I did this was that since I'm only passing a single array of pin numbers to the AdvancedADCDual.begin() I was not sure how else to split this up. If you want to add an additional array so that there is one array of pins per ADC, that would work too. I was trying to keep the complexity of usage down. You would also need two num_channels to denote the length of each array of pin numbers. I only did it to limit the number of parameters being passed to the begin routine. Its easy change. I guess you could split the array in half and assign half to one ADC and half to the other. But then the num_channels needs to be even, or the remainder is always assigned to one of the other ADC. For example, num_channels=5. Assign num_channels>>1=2 of the pins to ADC1, and num_channels-(num_channels>>1)=3 to the other ADC. |
I don't think we need to do that, see the update API. |
We can't assign pins dynamically with this, but I don't think we need to support this for dual ADCs. |
Yes. You could make it a requirement that pins are assigned upfront to each instance of AdvancedADC prior to calling the AdvancedADCDual.begin(). This solves the problem as well. I'll need to make a few minor changes to my code because I am calling it incorrectly. I'll make the changes to my app and see if everything works. |
No problem, take your time. Note there's an example here, but if you have a more useful one please send it. |
I'm having a real problem with the refactored code. I'm finding it difficult to declare an instance of class osc{
private:
uint16_t sample_period;
uint8_t probes[MAX_NUM_PROBES];
uint16_t num_samples;
uint32_t sample_rate;
bool dual_mode;
uint32_t timestamp;
uint32_t sampleDelay;
public:
uint32_t max_num_pins;
AdvancedADC adc_input[2];
AdvancedADCDual *adc_dual;
osc(uint8_t *p,uint8_t ena, uint16_t ns,uint32_t sr)
{
max_num_pins=MAX_NUM_PROBES;
for(uint8_t i=0;i<MAX_NUM_PROBES;i++)
{
probes[i]=p[i];
adc_input[i]=AdvancedADC(p[i]);
}
adc_dual=new AdvancedADCDual(adc_input[0],adc_input[1]); It compiles with no errors. Calling the I'm going to try running your example and see if that works for me. |
Your example works fine. When I globally declare the AdvancedADC instances and the AdvancedADCDual instance in a similar way my app also works fine as well. So, I must have some problem with how I'm declaring or using the AdvancedADCDual. So, I would say the refactored code is fine. I've just got to get a little more clever about how I use the API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
begin()
should return the status of initialization and not the id()
of the ADC.
Below my proposal
0b6f3fb
to
e89b1ed
Compare
@leonardocavagnis updated. |
Memory usage change @ e89b1ed
Click for full report table
Click for full report CSV
|
Fixes #54
Fixes #59