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 |
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 |
| Event13 = 0x0Du, | ||
| Event14 = 0x0Eu, | ||
| Event15 = 0x0Fu, | ||
| %% if target["family"] in ["g4"] |
There was a problem hiding this comment.
| %% if target["family"] in ["g4"] | |
| %% if target["family"] in ["g4", "h7"] |
H7 also has 32 trigger events
| 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.
| * refer to the ADC external trigger section on reference manual | ||
| * of your controller for more information | ||
| */ | ||
| enum class RegularConversionExternalTrigger |
There was a problem hiding this comment.
This struct is used for both regular and injected conversions. I'd rename it to just ExternalTriggerSource without the RegularConversion part.
| const uint32_t channelMask = (static_cast<uint32_t>(channel) << ADC_OFR1_OFFSET1_CH_Pos) & ADC_OFR1_OFFSET1_CH_Msk; | ||
| const uint32_t offsetValue = channelMask | offsetMask | enableMask | saturateMask | signMask; | ||
|
|
||
| if ((ADC{{id}}->CR & ADC_CR_JADSTART) || (ADC{{id}}->CR & ADC_CR_ADSTART)) |
There was a problem hiding this comment.
This reads the CR register twice. It can be done in one read:
| if ((ADC{{id}}->CR & ADC_CR_JADSTART) || (ADC{{id}}->CR & ADC_CR_ADSTART)) | |
| if (ADC{{id}}->CR & (ADC_CR_JADSTART | ADC_CR_ADSTART)) |
| bool | ||
| modm::platform::Adc{{ id }}::disableChannelOffset(const OffsetSlot slot) | ||
| { | ||
| if ( (ADC{{ id }}->CR & ADC_CR_JADSTART) || (ADC{{ id }}->CR & ADC_CR_ADSTART) ) { |
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.
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.