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 ADC support for WB55 #20773

Merged
merged 2 commits into from
Jul 8, 2024
Merged

Conversation

crasbe
Copy link
Contributor

@crasbe crasbe commented Jul 3, 2024

Contribution description

As described in the issue #17260, the ADC in the STM32WB55 has the same basic peripheral as the STM32L4 series, therefore the ADC implementation can be shared between these devices.

Edit: During the creation of this PR I found an error in the P-NUCLEO-WB55 user manual: https://www.st.com/resource/en/user_manual/um2435-bluetooth-low-energy-and-802154-nucleo-pack-based-on-stm32wb-series-microcontrollers-stmicroelectronics.pdf
In Table 10 on page 39, ADC1_IN5 and ADC1_IN6 are switched.

The datasheet is correct: https://www.st.com/resource/en/datasheet/stm32wb55cc.pdf Table 17 page 69.

Testing procedure

The tests/periph/adc test should run successfully on a P-NUCLEO-WB55 board with this PR applied (and #20756).

To compile the test I ran the following command:

BOARD=p-nucleo-wb55 make flash term

And this is the console output. The correct ADC lines can be verified by pulling the A0-A5 pins to GND and +3V3 respectively. To test the VBAT channel, SB30 has to be closed (or you can touch SB30 with a jumper cable which is connected to +3V3 on the other side).

main(): This is RIOT! (Version: 2024.07-devel-10-g41921a-pr/wb55_adc)

RIOT ADC peripheral driver test

This test will sample all available ADC lines once every 100ms with
a 10-bit resolution and print the sampled results to STDIO


Successfully initialized ADC_LINE(0)
Successfully initialized ADC_LINE(1)
Successfully initialized ADC_LINE(2)
Successfully initialized ADC_LINE(3)
Successfully initialized ADC_LINE(4)
Successfully initialized ADC_LINE(5)
Successfully initialized ADC_LINE(6)
ADC_LINE(0): 292
ADC_LINE(1): 339
ADC_LINE(2): 373
ADC_LINE(3): 396
ADC_LINE(4): 469
ADC_LINE(5): 378
ADC_LINE(6): 179
ADC_LINE(0): 290
ADC_LINE(1): 345
ADC_LINE(2): 368
ADC_LINE(3): 396
ADC_LINE(4): 426
ADC_LINE(5): 369
ADC_LINE(6): 185
ADC_LINE(0): 290
ADC_LINE(1): 347
ADC_LINE(2): 367
ADC_LINE(3): 397
ADC_LINE(4): 412
ADC_LINE(5): 369
ADC_LINE(6): 186

Issues/PRs references

Fixes #17260.
Depends on #20756, because with the mainline RIOT code, the ADC initialization does not work.
This will require a rebase when #20756 is merged because this PR changes the name of adc_l4.c to adc_l4_wb.c

@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports labels Jul 3, 2024
Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

I have the board somewhere at home. I can give it a try this weekend.

Comment on lines +64 to +66
ADC_RES_12BIT = (0x0), /**< ADC resolution: 12 bit */
ADC_RES_14BIT = (0x1), /**< not applicable */
ADC_RES_16BIT = (0x2) /**< not applicable */
Copy link
Member

Choose a reason for hiding this comment

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

driver will reject the resolution if the value is 0x03 (dunno why 0x03 though) but it might be a good idea to use this value to reject these resolution and thus prevent misconfiguration.

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe update the driver to use these values instead to reject the configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

driver will reject the resolution if the value is 0x03 (dunno why 0x03 though) but it might be a good idea to use this value to reject these resolution and thus prevent misconfiguration.

That's not quite accurate, the driver will reject the resolution if the value can be masked by 0x03. This is true for the 0x01 and 0x02 values. In binary it makes more sense: The mask is 0b0011 and the illegal values are 0b0001 and 0b0010.
0b0011 & 0b0001 = 0b0001, 0b0011 & 0b0010 = 0b0010.
Since the resolution bits RES in the ADC_CFGR register are at position 3 and 4, this can be used to find the two illegal values.

IMO this is not a very pretty solution since it relies on magic numbers and it is not obviously documented. But it seems to be the same for all other ADC implementations as well. For now I wouldn't change it though.

(This part of the code is just copied from the cpu/stm32/include/periph/l4/periph_cpu.h file. It is the same for the L4 and WB55.)


STM32WB55 Reference Manual: https://www.st.com/resource/en/reference_manual/rm0434-multiprotocol-wireless-32bit-mcu-armbased-cortexm4-with-fpu-bluetooth-lowenergy-and-802154-radio-solution-stmicroelectronics.pdf page 489

Copy link
Member

Choose a reason for hiding this comment

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

Then you could borrow the behavior from here which is more easy to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then you could borrow the behavior from here which is more easy to understand.

I generally agree with you, but I didn't change anything in the cpu/stm32/periph/adc_l4.c file in this PR. I just renamed it and changed the cpu/stm32/include/wb/periph_cpu.h file to be compatible with the data structures that adc_l4.c expects.

A better approach would be to create a new issue/PR to fix this issue for the other boards in one go, so we can keep things separate:
https://github.com/RIOT-OS/RIOT/blob/master/cpu/stm32/periph/adc_f0_g0_c0.c#L103-L106
https://github.com/RIOT-OS/RIOT/blob/master/cpu/stm32/periph/adc_f2.c#L112-L114
https://github.com/RIOT-OS/RIOT/blob/master/cpu/stm32/periph/adc_f3.c#L196-L199
https://github.com/RIOT-OS/RIOT/blob/master/cpu/stm32/periph/adc_f4_f7.c#L131-L134
https://github.com/RIOT-OS/RIOT/blob/master/cpu/stm32/periph/adc_wl.c#L121-L124

Better examples that don't rely on magic numbers:
https://github.com/RIOT-OS/RIOT/blob/master/cpu/stm32/periph/adc_f1.c#L146-L149
https://github.com/RIOT-OS/RIOT/blob/master/cpu/stm32/periph/adc_l0.c#L120-L126
https://github.com/RIOT-OS/RIOT/blob/master/cpu/stm32/periph/adc_l1.c#L149-L155

@crasbe
Copy link
Contributor Author

crasbe commented Jul 4, 2024

I have the board somewhere at home. I can give it a try this weekend.

Thank you. Remember that this PR depends on #20756, so you have to apply the changes of that PR to the adc_l4_wb.c file.

@dylad dylad added State: waiting for other PR State: The PR requires another PR to be merged first and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Jul 4, 2024
@dylad
Copy link
Member

dylad commented Jul 5, 2024

Feel free to rebase (ans squash the new extra commit) whenever you want.

@crasbe
Copy link
Contributor Author

crasbe commented Jul 5, 2024

Feel free to rebase (ans squash the new extra commit) whenever you want.

Done. This was a learning experience. Apparently it is not a good idea to use the convenient "sync" button in Github, because it leaves this merge commit and now all the commits that happened since are in the commit history. And merge commits can not be squashed 🙃
I reverted that and did a simple git rebase master pr/wb55_adc and it worked beautifully.

Now the PR has the #20756 PR incorporated and you should be able to test it now as it is :)

Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

Tested on p-nucleo-wb55. I was only able to test GND and 3V3 levels but this is looking good.
thanks !

@dylad dylad added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 7, 2024
@riot-ci
Copy link

riot-ci commented Jul 7, 2024

Murdock results

✔️ PASSED

25b3a58 boards/p-nucleo-wb55: enable ADC support

Success Failures Total Runtime
10177 0 10178 13m:32s

Artifacts

@dylad dylad added this pull request to the merge queue Jul 7, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 7, 2024
@dylad
Copy link
Member

dylad commented Jul 8, 2024

@crasbe It seems CI catches an issue

@dylad
Copy link
Member

dylad commented Jul 8, 2024

At first glance, I would say 'wb' must be added there

@dylad
Copy link
Member

dylad commented Jul 8, 2024

please squash.

@dylad
Copy link
Member

dylad commented Jul 8, 2024

Round 2 !

@dylad dylad added this pull request to the merge queue Jul 8, 2024
Merged via the queue into RIOT-OS:master with commit e1e5f75 Jul 8, 2024
25 checks passed
@crasbe
Copy link
Contributor Author

crasbe commented Jul 8, 2024

Thank you @dylad for your comprehensive help :)

@dylad
Copy link
Member

dylad commented Jul 8, 2024

Thanks for your contribution :)

@crasbe crasbe deleted the pr/wb55_adc branch July 8, 2024 13:18
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 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.

boards/p-nucleo-wb55: add ADC configuration
4 participants