stm32-rs / stm32l4xx-hal

A Hardware abstraction layer for the stm32l432xx series chips written in rust.
156 stars 103 forks source link

Support multiple ADCs. #312

Closed reitermarkus closed 2 years ago

reitermarkus commented 2 years ago

Improve support for ADC2 and ADC3.

korken89 commented 2 years ago

Hi, thanks for improving the ADC support!

I have this a quick look and noticed a thing. The ADC_COMMON is no longer owned by the ADC which means it's still available as part of the peripherals structure. This can cause unintended consequences as users can access the ADC_COMMON after an ADC is constructed. This would need fixing to merge this.

I also noticed that you are constructing the ADC_COMMON when needed, this would be fine if you only did read or write atomic operations, however this is not the case as there are modify statements. As it is now each modify statement using ADC_COMMON is a race condition. This would also need to be solved.

Finally, I see you have extended unit structs with a _0 field, why is this? I specifically removed all those a long time ago when the compiler got support for unit structs.

Keep up the good work!

reitermarkus commented 2 years ago

The ADC_COMMON is no longer owned by the ADC which means it's still available as part of the peripherals structure.

Yeah, I was wondering about the same thing with ADC_COMMON. I took the F4 Adc implementation as a template, which does the same thing with ADC_COMMON.

So we should instead pass it everywhere it is needed explicitly as &mut ADC_COMMON instead, right? Alternatively, consume it into an AdcCommon struct, behaving similar to Clocks, which cannot be released again and protect the modifys with interrupt::free.

Finally, I see you have extended unit structs with a _0 field, why is this?

To calibrate e.g. ADC1, you have to enable VREF, so it makes sense that you can only get a Vref by calling enable_vref. The same goes for measuring Vref, Vbat and Temperature. I was also debating whether you should then have to pass the Vref back to disable_vref since you could theoretically enable it then disable it and still have the owned Vref.

reitermarkus commented 2 years ago

Then again, you can just call enable_vref twice and have two owned Vrefs, so it doesn't work quite like I imagined.

The F4 implementation checks internally in the calibrate function if it is enabled and temporarily enables it if it isn't.

reitermarkus commented 2 years ago

I created an AdcCommon wrapper, which also has the side effect of replacing the reset parameter when constructing an Adc, since constructing an AdcCommon will enable and reset it.

reitermarkus commented 2 years ago

@korken89, can you have another look? Thanks.

reitermarkus commented 2 years ago

@korken89, can you have another look? Thanks.

reitermarkus commented 2 years ago

@korken89, ping.

reitermarkus commented 2 years ago

@korken89, can you have another look? Thanks.

korken89 commented 2 years ago

This LGTM, thanks for the effort! @MathiasKoch feel free to merge when you are also happy.

MathiasKoch commented 2 years ago

:tada: