Skip to content

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Nov 21, 2024

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 the BAT registry entry.

  • feather-m0, feather-m0-lora, or feather-m0-wifi

Issues/PRs references

Based on #21018

@github-actions github-actions bot added Area: build system Area: Build system Area: drivers Area: Device drivers Area: boards Area: Board ports Area: SAUL Area: Sensor/Actuator Uber Layer labels Nov 21, 2024
Copy link
Contributor

@crasbe crasbe left a 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.

@crasbe crasbe added the Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation label May 16, 2025
@miri64 miri64 force-pushed the saul_bat_voltage/enh/feather-m0-config branch 2 times, most recently from cb3cbc6 to 1b578fc Compare May 20, 2025 11:42
@github-actions github-actions bot removed the Area: build system Area: Build system label May 20, 2025
@miri64 miri64 force-pushed the saul_bat_voltage/enh/feather-m0-config branch from abb0908 to fbe894f Compare May 20, 2025 11:49
@crasbe crasbe added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 22, 2025
@riot-ci
Copy link

riot-ci commented May 22, 2025

Murdock results

✔️ PASSED

fbe894f feather-m0: add support for saul_bat_voltage

Success Failures Total Runtime
10363 0 10363 12m:37s

Artifacts

@miri64
Copy link
Member Author

miri64 commented May 23, 2025

👍 now only some actual tests are needed 😅

@mguetschow
Copy link
Contributor

👍 now only some actual tests are needed 😅

didn't you have such hardware at home?

@crasbe crasbe marked this pull request as ready for review May 23, 2025 13:52
@crasbe
Copy link
Contributor

crasbe commented May 23, 2025

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.
One of these days I'll remember to bring the Feather to work and test it there... hopefully 🫠

@crasbe
Copy link
Contributor

crasbe commented May 23, 2025

Good news: I just remembered that my server runs Ubuntu Server and I can use that to test the PR.
Bad news: it doesn't work 😅

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.

2025-05-23 17:11:34,132 # -gfbe894-saul_bat_voltage/enh/feather-m0-config)
2025-05-23 17:11:34,133 # Welcome to RIOT!
2025-05-23 17:11:34,133 #
2025-05-23 17:11:34,136 # Type `help` for help, type `saul` to see all SAUL devices
2025-05-23 17:11:34,137 #
> help
2025-05-23 17:11:36,357 # help
2025-05-23 17:11:36,359 # Command              Description
2025-05-23 17:11:36,361 # ---------------------------------------
2025-05-23 17:11:36,364 # pm                   interact with layered PM subsystem
2025-05-23 17:11:36,367 # ps                   Prints information about running threads.
2025-05-23 17:11:36,370 # saul                 interact with sensors and actuators using SAUL
2025-05-23 17:11:36,372 # version              Prints current RIOT_VERSION
2025-05-23 17:11:36,374 # bootloader           Reboot to bootloader
2025-05-23 17:11:36,376 # reboot               Reboot the node
> saul
2025-05-23 17:11:37,289 # saul
2025-05-23 17:11:37,290 # ID    Class           Name
2025-05-23 17:11:37,291 # #0    SENSE_VOLTAGE   BAT
2025-05-23 17:11:37,292 # #1    ACT_SWITCH      LED(Red)
> saul read 0
2025-05-23 17:11:41,699 # saul read 0
2025-05-23 17:11:41,701 # Reading from #0 (BAT|SENSE_VOLTAGE)
2025-05-23 17:11:41,703 # Data:              -6 mV
saul read 0
2025-05-23 17:13:34,444 # saul read 0
2025-05-23 17:13:34,446 # Reading from #0 (BAT|SENSE_VOLTAGE)
2025-05-23 17:13:34,448 # Data:              -6 mV
> 

@crasbe
Copy link
Contributor

crasbe commented May 23, 2025

I just ran the tests/periph/adc test and this is the output from connecting BAT to GND with an 82 Ohm resistor (I totally didn't try it with a jumper and crash the board 👀 ) and to +3.3V.

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.
-1 * 2 * 3300 / 1024 = -6.44 ~ -6

2025-05-23 17:18:21,092 # ADC_LINE(0): -1 255 1023 4095    -1 65520
2025-05-23 17:18:21,095 # ADC_LINE(1): -1 255 1023 4095    -1 65520
2025-05-23 17:18:21,097 # ADC_LINE(2): -1 255 1023 4095    -1 65520
2025-05-23 17:18:21,099 # ADC_LINE(3): -1 255 1023 4095    -1 65520
2025-05-23 17:18:21,101 # ADC_LINE(4): -1 255 1023 4095    -1 65520
2025-05-23 17:18:21,103 # ADC_LINE(5): -1 255 1023 4095    -1 65520
2025-05-23 17:18:21,105 # ADC_LINE(6): -1  84  338 1348    -1 21622
2025-05-23 17:19:53,376 #
2025-05-23 17:19:53,477 # ADC_LINE(0): -1 255 1023 4095    -1 65520
2025-05-23 17:19:53,479 # ADC_LINE(1): -1 255 1023 4095    -1 65520
2025-05-23 17:19:53,481 # ADC_LINE(2): -1 255 1023 4095    -1 65520
2025-05-23 17:19:53,483 # ADC_LINE(3): -1 255 1023 4095    -1 65520
2025-05-23 17:19:53,485 # ADC_LINE(4): -1 255 1023 4095    -1 65520
2025-05-23 17:19:53,487 # ADC_LINE(5): -1 255 1023 4095    -1 65520
2025-05-23 17:19:53,489 # ADC_LINE(6): -1 255 1023 4095    -1 65520

{
.name = "BAT",
.phydat_scale = -3,
.line = ADC_LINE(7),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.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.

@crasbe
Copy link
Contributor

crasbe commented May 23, 2025

More testing, now with lab supply and Multimeter:
Real Voltage: 0.81V
Saul Readout: 2.68V

Real Voltage: 1.29V
Saul Readout: 4.28V

Real Voltage: 1.60V
Saul Readout: 5.30V

Real Voltage: 1.98V
Saul Readout: 6.53V (About at the end of the scale)

Real Voltage: 4.21V
Saul Readout: 6.59V (end of scale)

@miri64
Copy link
Member Author

miri64 commented May 24, 2025

More testing, now with lab supply and Multimeter: Real Voltage: 0.81V Saul Readout: 2.68V

Real Voltage: 1.29V Saul Readout: 4.28V

Real Voltage: 1.60V Saul Readout: 5.30V

Real Voltage: 1.98V Saul Readout: 6.53V (About at the end of the scale)

Real Voltage: 4.21V Saul Readout: 6.59V (end of scale)

That seems too big. :-/

@miri64
Copy link
Member Author

miri64 commented May 24, 2025

So why did you move this PR out of draft stage? ;-)

@miri64 miri64 added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label May 24, 2025
* we return in millivolt so we set the reference voltage to 3300
* instead of 3.3
*/
return (int16_t)((adc_sample * 2L * 3300L) / 1024L);
Copy link
Member Author

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:

Suggested change
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.

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is here:

/* 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... 🤷

@crasbe
Copy link
Contributor

crasbe commented May 24, 2025

So why did you move this PR out of draft stage? ;-)

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.

@crasbe crasbe added the State: waiting for other PR State: The PR requires another PR to be merged first label May 24, 2025
@crasbe
Copy link
Contributor

crasbe commented May 24, 2025

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:

2025-05-24 16:59:10,998 # -g7efa8f-saul_bat_voltage/enh/feather-m0-config)
2025-05-24 16:59:11,000 # Welcome to RIOT!
2025-05-24 16:59:11,000 #
2025-05-24 16:59:11,003 # Type `help` for help, type `saul` to see all SAUL devices
2025-05-24 16:59:11,003 #
> saul read 0
2025-05-24 16:59:13,813 # saul read 0
2025-05-24 16:59:13,816 # Reading from #0 (BAT|SENSE_VOLTAGE)
2025-05-24 16:59:13,817 # Data:             825 mV
>

However one thing I wondered in general: Why does this approach not use the periph_vbat feature that the STM32 Nucleos use?

@crasbe crasbe removed the State: waiting for other PR State: The PR requires another PR to be merged first label May 24, 2025
@miri64
Copy link
Member Author

miri64 commented May 26, 2025

However one thing I wondered in general: Why does this approach not use the periph_vbat feature that the STM32 Nucleos use?

Because I, and apparently the reviewers of #21018, did not know about it 😅

@miri64
Copy link
Member Author

miri64 commented May 26, 2025

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:

2025-05-24 16:59:10,998 # -g7efa8f-saul_bat_voltage/enh/feather-m0-config)
2025-05-24 16:59:11,000 # Welcome to RIOT!
2025-05-24 16:59:11,000 #
2025-05-24 16:59:11,003 # Type `help` for help, type `saul` to see all SAUL devices
2025-05-24 16:59:11,003 #
> saul read 0
2025-05-24 16:59:13,813 # saul read 0
2025-05-24 16:59:13,816 # Reading from #0 (BAT|SENSE_VOLTAGE)
2025-05-24 16:59:13,817 # Data:             825 mV
>

Is that the expected output?

@miri64
Copy link
Member Author

miri64 commented May 26, 2025

However one thing I wondered in general: Why does this approach not use the periph_vbat feature that the STM32 Nucleos use?

Because I, and apparently the reviewers of #21018, did not know about it 😅

Also: Is it exposed via SAUL? For my use-case (advertising the battery voltage via skald_bthome), I need that.

@crasbe
Copy link
Contributor

crasbe commented May 27, 2025

Is that the expected output?

Yes.

However one thing I wondered in general: Why does this approach not use the periph_vbat feature that the STM32 Nucleos use?

Because I, and apparently the reviewers of #21018, did not know about it 😅

Also: Is it exposed via SAUL? For my use-case (advertising the battery voltage via skald_bthome), I need that.

Not yet, because the VBat of the STM32s is more for battery backup of the RTC etc.
You can read the voltage, but it doesn't have as much of a practical usecase as the Lipo monitor of the Feathers.

@miri64
Copy link
Member Author

miri64 commented May 27, 2025

Not yet, because the VBat of the STM32s is more for battery backup of the RTC etc. You can read the voltage, but it doesn't have as much of a practical usecase as the Lipo monitor of the Feathers.

So that is another reason :-) But I mean, there is nothing preventing us from merging the two features in the future.

@crasbe
Copy link
Contributor

crasbe commented Jun 4, 2025

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 periph_vbat.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: drivers Area: Device drivers Area: SAUL Area: Sensor/Actuator Uber Layer CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants