Skip to content
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

cpu/stm32: add backup battery monitoring (VBAT) #16989

Merged
merged 6 commits into from
Feb 21, 2022

Conversation

fabian18
Copy link
Contributor

Contribution description

This PR adds a driver to monitor the backup battery status for many stm32 based boards with ADC.
To check which boards support battery monitoring, I did:
grep -I -r '_VBAT' | cut -d' ' -f1 | sort | uniq in cmsis-header-stm32.
As a reference of how to calculate the voltage value from the ADC sample I found this.

Testing procedure

I tested tests/driver_vbat with nucleo-f767zi.

2021-10-14 17:36:51,385 # main(): This is RIOT! (Version: 2021.10-devel-960-g0ec3a-cpu_stm32_add_vbat)
2021-10-14 17:36:51,385 # 
2021-10-14 17:36:51,388 # RIOT backup battery monitoring test
2021-10-14 17:36:51,388 # 
2021-10-14 17:36:51,393 # This test will sample the backup battery once a second
2021-10-14 17:36:51,393 # 
2021-10-14 17:36:51,394 # 
2021-10-14 17:36:51,395 # VBAT: 2984[mV]
2021-10-14 17:36:52,397 # VBAT: 3233[mV]
2021-10-14 17:36:53,399 # VBAT: 3246[mV]
2021-10-14 17:36:54,400 # VBAT: 3210[mV]
2021-10-14 17:36:55,402 # VBAT: 3178[mV]
2021-10-14 17:36:56,404 # VBAT: 3216[mV]
2021-10-14 17:36:57,406 # VBAT: 3207[mV]
2021-10-14 17:36:58,408 # VBAT: 3213[mV]

Issues/PRs references

This PR includes #16870 and was created to have battery monitoring independent.

@github-actions github-actions bot added Area: boards Area: Board ports Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: tests Area: tests and testing framework Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Oct 14, 2021
@benpicco benpicco added the State: waiting for other PR State: The PR requires another PR to be merged first label Oct 14, 2021
@benpicco
Copy link
Contributor

This needs a rebase 😉

@benpicco benpicco removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jan 21, 2022
@github-actions github-actions bot added the Area: doc Area: Documentation label Jan 23, 2022
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Sorry for losing track of this again!
Looks good in general, I like a dedicated, simple API for something simple as opposed to a big generic API that makes the simple things hard.

It would be a laudable task to unify the stm32 ADC implementations, maybe adc_ng will get us there.

A bit of bikeshedding ahead 😉

stm32wl% \

ifneq (,$(filter $(STM32_WITH_VBAT),$(CPU_MODEL)))
FEATURES_PROVIDED += vbat
Copy link
Contributor

Choose a reason for hiding this comment

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

since you started that bikeshedding last time 😉

Why not periph_vbat?
Battery monitoring seems to be more a peripheral thing than battery RAM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name periph_vbat is surely comprehensible. But I personally would not call it like that because probably no datasheet would list it as an extra peripheral because it is a feature of the ADC. I saw that there is cpu/efm32/drivers/coretemp/coretemp.c which is also samples an ADC channel. So I decided to implement VBAT similar to this. So you think it should rather be periph_vbat?

Copy link
Contributor

Choose a reason for hiding this comment

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

Although that is efm32_coretemp (and I think a generic version should be periph_coretemp).

Generally I understand the periph_ prefix as something that needs a per-CPU implementation as opposed to a generic module. (I see that this makes the case for periph_backup_ram…)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I think it should just be coretemp 😅. Because if my use case or application requires the core temperature, I´d rather do USEMODULE += coretemp and not care if my CPU is efm32. Or better, coretem could pull in efm32_coretemp for efm32 but that´s not the case right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes or periph_coretemp like you said.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyway, I have no problem with periph_, if you think it´s better.

Comment on lines 35 to 51
/* f0 */
#if defined(CPU_LINE_STM32F031x6) || defined(CPU_LINE_STM32F038xx) || \
defined(CPU_LINE_STM32F042x6) || defined(CPU_LINE_STM32F048xx) || \
defined(CPU_LINE_STM32F051x8) || defined(CPU_LINE_STM32F058xx) || \
defined(CPU_LINE_STM32F071xB) || defined(CPU_LINE_STM32F072xB) || \
defined(CPU_LINE_STM32F078xx) || defined(CPU_LINE_STM32F091xC) || \
defined(CPU_LINE_STM32F098xx)
#define VBAT_ADC_SCALE 2
#define VBAT_ADC_MIN_MV 1650
#define ADC_CCR_REG (ADC->CCR)
/* f2 */
#elif defined(CPU_LINE_STM32F205xx) || defined(CPU_LINE_STM32F207xx) || \
defined(CPU_LINE_STM32F215xx) || defined(CPU_LINE_STM32F217xx)
#define VBAT_ADC_SCALE 2
#define VBAT_ADC_MIN_MV 1800
#define ADC_CCR_REG (ADC->CCR)
/* f3 */
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would better belong to cpu/stm32/include/periph/<line>/periph_cpu.h but if you prefer it here I won't argue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since nothing else uses it, I´d keep it there.

tests/driver_vbat/main.c Outdated Show resolved Hide resolved
cpu/stm32/drivers/vbat/vbat.c Outdated Show resolved Hide resolved
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 19, 2022
boards/nucleo-f767zi/include/periph_conf.h Outdated Show resolved Hide resolved
cpu/stm32/cpu_init.c Outdated Show resolved Hide resolved
cpu/stm32/periph/adc_f0_g0.c Show resolved Hide resolved
@benpicco
Copy link
Contributor

Murdock found some issues

@fabian18
Copy link
Contributor Author

When doing TEST_KCONFIG=1 BOARD=nucleo-f767zi make -C tests/periph_vbat menuconfig, VBAT_ADC_VREF_MV does not appear anywhere. I have not figured out why, yet.

@benpicco
Copy link
Contributor

Not sure, maybe @leandrolanzieri has an idea.
Murdock appears to be happy though - please squash.

Comment on lines 29 to 38
if KCONFIG_USEMODULE_PERIPH_VBAT

config VBAT_ADC_VREF_MV
int "ADC reference voltage in mV"
default 3300
help
This is the reference voltage (VREF) of the ADC.
Often VREF is connected with VDDA, which is equal to VDD.

endif # KCONFIG_USEMODULE_PERIPH_VBAT
Copy link
Contributor

Choose a reason for hiding this comment

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

The reason it does not show up when TEST_KCONFIG=1 is that, when that flag is active, Makefile dependency resolution does not run -> USEMODULE_PERIPH_VBAT is not defined. Although it implies some duplication, I'd go with the option of defining it again, in the first menuconfig. It could look something like:

menuconfig MODULE_PERIPH_VBAT
    bool "Backup Battery monitoring driver"
    depends on HAS_PERIPH_VBAT
    depends on HAS_PERIPH_ADC
    depends on TEST_KCONFIG
    select MODULE_PERIPH_ADC

if MODULE_PERIPH_VBAT

config MODULE_PERIPH_INIT_VBAT
    bool "Auto initialize VBAT ADC line"
    default y if MODULE_PERIPH_INIT

config VBAT_ADC_VREF_MV
    int "ADC reference voltage in mV"
    default 3300

endif # MODULE_PERIPH_VBAT

menuconfig KCONFIG_USEMODULE_PERIPH_VBAT
    bool "Configure backup battery monitoring peripheral driver"
    depends on USEMODULE_PERIPH_VBAT
    help
        Configure backup battery monitoring peripheral driver using Kconfig.

config VBAT_ADC_VREF_MV
    int "ADC reference voltage in mV"
    default 3300
    depends on KCONFIG_USEMODULE_PERIPH_VBAT
    help
        This is the reference voltage (VREF) of the ADC.
        Often VREF is connected with VDDA, which is equal to VDD.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I am going to squash that in right away.

@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 21, 2022
@benpicco benpicco merged commit f966dfa into RIOT-OS:master Feb 21, 2022
@fabian18
Copy link
Contributor Author

Thank you.

@OlegHahm OlegHahm added this to the Release 2022.04 milestone Apr 25, 2022
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: cpu Area: CPU/MCU ports Area: doc Area: Documentation Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants