Feature: STM32 ADC trigger and offsets api#1275
Conversation
|
Thanks! Could you Squash your commits, rebase on develop and remove the CMake part that's already in the other PR? I can also do it if you want. |
|
Ur welcome. Let me try it first. I need to practice this. I still want to add API and test it. I just wanted to get the pipeline running to see if I overlooked something. |
Tecnologic
force-pushed
the
feature/adc_trigger_api
branch
3 times, most recently
from
dc43c82 to
080c495
Compare
Tecnologic
force-pushed
the
feature/adc_trigger_api
branch
10 times, most recently
from
8f47250 to
ef28598
Compare
|
I need to retest this with the last fixes on my custom stm32g473 board and i may be able to test this on a nucleo-H7a3 board. For other stm32 on which this is relevant i have no hardware available |
salkinium
left a comment
salkinium
left a comment
There was a problem hiding this comment.
Nice! The CI is finally passing, yay! \o/
Tecnologic
force-pushed
the
feature/adc_trigger_api
branch
from
ef28598 to
d771a5c
Compare
|
How is a test for this api supposed to be? Should i write a g4 nucleo example or something in the unittests area? |
Tecnologic
force-pushed
the
feature/adc_trigger_api
branch
from
d771a5c to
608f8a2
Compare
|
You can add an example if you like, but I'm also happy to merge it as is. The advantage of adding an example is that breaking API changes will be detected in the CI. The unit tests are not currently executed on hardware (only manually every now and then), so there's less guarantee. |
|
As I have no time for the next weeks to continue. I'll skip the example. And just leake it like it is |
| modm::platform::Adc{{ id }}::enableChannelOffset( const OffsetSlot slot, const Channel channel, const int16_t offset, const bool saturate) | ||
| { | ||
| const uint32_t offsetMask = (std::abs(offset) << ADC_OFR1_OFFSET1_Pos) & ADC_OFR1_OFFSET1_Msk; | ||
| %% if target["family"] in["h7"] | ||
| %# H7 names the bits differently, but the logic is the same | ||
| const uint32_t signMask = (offset > 0) ? ADC3_OFR1_OFFSETPOS : 0u; | ||
| const uint32_t saturateMask = saturate ? ADC3_OFR1_SATEN : 0u; | ||
| %% else | ||
| const uint32_t signMask = (offset > 0) ? ADC_OFR1_OFFSETPOS : 0u; | ||
| const uint32_t saturateMask = saturate ? ADC_OFR1_SATEN : 0u; | ||
| %% endif | ||
| %% endif |
There was a problem hiding this comment.
This doesn't work correctly on H72x/73x devices which contain two completely different ADC implementations in one chip. ADCs 1/2 are 16 bit ADCs identical to the ones in other H7 devices whereas ADC3 is a 12-bit ADC very similar to the one in STM32G4.
That is the reason why the headers have both ADC_* and ADC3_* register definitions. The correct way to handle this is to differentiate the ADCs by resolution. The 16 bit ones share the same implementation on all H7s and the 12 bit ADC uses the same code as the G4 one. See here for an example.
@Tecnologic Would you like to fix it or should I?
There was a problem hiding this comment.
Oh, that explains a lot. I'm not at home for the next weeks. So not able to fix. I if you don't mind, I'm glad when you fix it. Thanks.
There was a problem hiding this comment.
Hi Chris. How can i check for the adcs resulution?
There was a problem hiding this comment.
is this implementation then only pulled in for the 12Bit adc or the 16bit as well? I will try to push a draft and see what ci says.
|
FYI: I'm going to merge this as is on Monday. |
|
Great, Stil in Holidays. :) |
|
FYI2: I'm going to merge this as is in one week. ;-p |
|
I've fixed the issue with H7 devices and applied the review changes here, but I'm not allowed to push it to the PR. @Tecnologic Could you enable the option to allow maintainers to edit the PR? |
We're can I do that. I just have my phone here |
|
I don't find the option GitHub describes in its docs. If it's ok a can do the changes on the 15th of September. |
It apparently doesn't work if the PR is created from an organisation account: We can continue when you are back. Enjoy your vacation :) |
Tecnologic
force-pushed
the
feature/adc_trigger_api
branch
4 times, most recently
from
3ab737b to
32a245d
Compare
|
I had a look at all the H7 headers, and i think i understood the concern of @chris-durand so lets see how this works in CI. |
Tecnologic
force-pushed
the
feature/adc_trigger_api
branch
from
32a245d to
647c16a
Compare
|
Hey @Tecnologic, welcome back :). The It would be very bug-prone if the saturate parameter did vastly different things on different platforms. Also, the 16-bit ADC only supports positive offsets. That's why I have grouped H7 16-bit ADC together with F3, L4 and L5 in my proposed changes. The downside is that there is no support for Could you take a look at my changes again? It probably would be easier to just push them here and go from there. They should compile. |
Tecnologic
force-pushed
the
feature/adc_trigger_api
branch
from
647c16a to
fd335d5
Compare
|
Hi chris, thanks for the link. then viewing the hole file i saw that i missunderstood what you were refering to. Good to know that the 16bit saturation bit is so missleading. i have not used these adc's up until now and wasn't aware of it |
|
I'll try the changes on a H723 Nucleo tomorrow |
The offset is working for both the 12 and 16 bit ADCs |
chris-durand
added
ci:hal
|
@Tecnologic Would you be fine with merging the PR in its current state or is there anything you'd like to change? |
|
I'm fine if h7 is working and you guys are fine with it. |
|
Thanks! |
|
Welcome, appreciate your work! |
3 participants
Problem:
API for setting the conversion triggers and Channel offsets are missing.
Changes
Provide API to set the regular and injected conversion trigger as well as setting offsets for adc chanels.