-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feather-m0: add support for saul_bat_voltage #21022
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
base: master
Are you sure you want to change the base?
feather-m0: add support for saul_bat_voltage #21022
Conversation
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.
Some small nitpicks, otherwise this looks very good.
It will require a rebase after #21349.
cb3cbc6
to
1b578fc
Compare
Co-authored-by: crasbe <[email protected]>
Co-authored-by: crasbe <[email protected]>
Co-authored-by: crasbe <[email protected]>
Co-authored-by: crasbe <[email protected]>
Co-authored-by: crasbe <[email protected]>
abb0908
to
fbe894f
Compare
👍 now only some actual tests are needed 😅 |
didn't you have such hardware at home? |
I have a Feather-M0 at home, but it's difficult to flash with WSL and even converting the .hex to a .uf2 file worked, but I can't access the serial terminal. |
Good news: I just remembered that my server runs Ubuntu Server and I can use that to test the PR. It always shows -6mV, regardless of whether the BAT pin is left floating (and the charge controller just seems to cycle, at least the LED is blinking very fast) or connected to 3.3V, which makes the LED light up permanently.
|
I just ran the Two things are evident here: connecting it to 3.3V does not show anything, therefore the range of the battery reading might be incorrect (wrong AREF perhaps?) and two resolutions show -1, which indicates that they are not supported.
|
{ | ||
.name = "BAT", | ||
.phydat_scale = -3, | ||
.line = ADC_LINE(7), |
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.
.line = ADC_LINE(7), | |
.line = ADC_LINE(6), |
While the battery is at (a fictional) A7, this is the sixth line. That explains the -1 return value.
More testing, now with lab supply and Multimeter: Real Voltage: 1.29V Real Voltage: 1.60V Real Voltage: 1.98V Real Voltage: 4.21V |
That seems too big. :-/ |
So why did you move this PR out of draft stage? ;-) |
* we return in millivolt so we set the reference voltage to 3300 | ||
* instead of 3.3 | ||
*/ | ||
return (int16_t)((adc_sample * 2L * 3300L) / 1024L); |
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.
There is a second formula in https://learn.adafruit.com/adafruit-feather-m0-basic-proto/power-management: (pin.value * 3.3) / 65536 * 2
Even though, if the resolution is correct, I don't think that should be it, @crasbe can you try with your setup:
return (int16_t)((adc_sample * 2L * 3300L) / 1024L); | |
return (int16_t)((adc_sample * 2L * 3300L) / 65536L); |
But maybe it is best to use your setup to have an output with the real voltage, the ADC readout and then reverse engineer the formular from that.
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.
I don't think that this is the right approach. The Feather-M0 has a voltage divider for the battery voltage, so that connecting 3.3V to the BAT input should absolutely not cause the reading to max out.
Also, this would be a reduction of ~64, whereas the real values are lower by a factor of (nearly exactly) 3.3.
Something is wrong with the settings for the ADC, but I didn't feel like diving into that yesterday. I have no experience with the SAMD21 yet.
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.
The issue is here:
RIOT/boards/feather-m0/include/periph_conf.h
Lines 185 to 190 in 584e14b
/* ADC Default values */ | |
#define ADC_PRESCALER ADC_CTRLB_PRESCALER_DIV512 | |
#define ADC_NEG_INPUT ADC_INPUTCTRL_MUXNEG_GND | |
#define ADC_GAIN_FACTOR_DEFAULT ADC_INPUTCTRL_GAIN_1X | |
#define ADC_REF_DEFAULT ADC_REFCTRL_REFSEL_INT1V |
The internal 1V reference is selected with a x1 gain. From the meausrements above it is clear that the ADC is out of range at 2V input at the battery pin. With a 1/2 divider, that's 1V => exactly what the reference voltage is.
What should be selected is the VCCA/2 reference with a 1/2 gain, so that the input does not exceed the reference voltage.
/* ADC Default values */
#define ADC_PRESCALER ADC_CTRLB_PRESCALER_DIV512
#define ADC_NEG_INPUT ADC_INPUTCTRL_MUXNEG_GND
-#define ADC_GAIN_FACTOR_DEFAULT ADC_INPUTCTRL_GAIN_1X
-#define ADC_REF_DEFAULT ADC_REFCTRL_REFSEL_INT1V
+#define ADC_GAIN_FACTOR_DEFAULT ADC_INPUTCTRL_GAIN_DIV2
+#define ADC_REF_DEFAULT ADC_REFCTRL_REFSEL_INTVCC1
The downside of this is that, contrary to the other analog inputs, the battery voltage does not depend on VCCA and therefore will only be as precise as the analog voltage (which is not really precise).
For a precise measurement, we'd have to measure the internal reference with respect to the VCCA.
Essentially this:
V_BAT = 2* (ADC_BAT / 1024) * 3300 * (ADC_Ref / 1024) * (3300 / 1000)
The first part is the same as previously, but then we factor in the ADC Reference voltage correction. The ADC_Ref value comes from 1V * 1/2 measured with a 1.65V reference, so if VCC was at a perfect 3300mV, the expression would be 1.
However I don't think that this mechanism would be supported at the moment and it probably goes too far as well.
Maybe it would be worth adding a note though that the measurement might be unprecise. On the other hand, that's also what all Arduinos do, so... 🤷
Because I don't consider this PR to be in the draft stage, it's very much close to completion and there is probably just an unrelated bug somewhere else. |
After rebasing to include #21508 and applying the suggestion to change the ADC channel from 7 to 6, this is what the output looks like:
However one thing I wondered in general: Why does this approach not use the |
Because I, and apparently the reviewers of #21018, did not know about it 😅 |
Is that the expected output? |
Also: Is it exposed via SAUL? For my use-case (advertising the battery voltage via |
Yes.
Not yet, because the VBat of the STM32s is more for battery backup of the RTC etc. |
So that is another reason :-) But I mean, there is nothing preventing us from merging the two features in the future. |
This has to be rebased after #21504. It's fine for me if (for the time being) we have two similar features, but I'd like to have to cross-referencing in the documentation and/or an issue to keep track of eventually merging them and/or adding SAUL for |
Contribution description
When trying to get #21018 to work for the
feather-m0
, the ADC line only returned-1
, so I removed this from #21018.Testing procedure
Flash
example/saul
to one (or ideally all) of the supported boards and use the CLI to read from theBAT
registry entry.Issues/PRs references
Based on #21018