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

fix scan mode for STM32F3 #977

Conversation

victorandrehc
Copy link
Contributor

@victorandrehc victorandrehc commented Mar 14, 2023

This PR removes enableScanMode and disableScanMode methods and incorporate them into addChannel.

  • STM32F3 family doesn't have a hardware Scan mode. Its scan mode is computed via ADCx->SQR1 L bits only. Methods enableScanMode and disableScanMode make little sense then, since the addChannle method already handles it.

@victorandrehc victorandrehc changed the title fix warning while compilling the scan mode fix scan mode for STM32F3 Mar 14, 2023
@victorandrehc victorandrehc marked this pull request as ready for review March 14, 2023 17:05
@victorandrehc victorandrehc force-pushed the fix/enable_scan_mode_overflow branch 2 times, most recently from 8f7bee5 to 9c1233f Compare March 14, 2023 20:38
@victorandrehc
Copy link
Contributor Author

victorandrehc commented Mar 15, 2023

@salkinium I created this PR improving the scan mode that was merged on monday, but it seems like the CI died on me. It made 13/25 tests and never executed the last 12. How can I make it execute the last 12?

@salkinium
Copy link
Member

The 25 other CI jobs are guarded behind the ci:hal tag, since they run quite long and "steal" CI time from other PRs. It's not the best mechanism (you cannot set the tag, only maintainers).
You can however execute the HAL tests locally, see the readme in the tests folder.

@salkinium salkinium added the ci:hal Triggers the exhaustive HAL compile CI jobs label Mar 15, 2023
@salkinium
Copy link
Member

I don't really know anything about the ADCs, so I'm delegating this to @rleh ;-P

@victorandrehc
Copy link
Contributor Author

This Pr basically sets the scan flag on register CR1 and remove the enable and disable scan mode methods, because I figured that if one is adding a channel they are probably meaning scan mode, so would be better to just enable scan mode there silently.

@rleh
Copy link
Member

rleh commented Mar 16, 2023

I'll look into it, give me some (2-3) days...

Copy link
Member

@chris-durand chris-durand left a comment

Choose a reason for hiding this comment

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

Hi @victorandrehc,

the F3 devices in the linked reference manual use a completely different ADC driver found in modm/platform/adc/stm32f3. The driver you changed is for F1, F2, F4, F7, L1 and F37x devices only. The scan mode flag seems to be present in all of them.

I'd be interested what your actual use-case for scan mode is. In my understanding scan mode is only valid when utilizing DMA which the driver does not support yet.

What it does is automatically starting the next conversion after the previous one has been completed. The DMA hardware would be triggered to fetch the conversion result and the next sampling cycle would commence shortly after. It's not recommended to use that mode with interrupts since the latency could be too high and the data register might be invalidated before it is read.

Thus, the F1 reference manual states:

scan

@victorandrehc
Copy link
Contributor Author

victorandrehc commented Mar 17, 2023

Hi @chris-durand, I will change(eliminate) the reference of the F3 then, I thought it was referring to F303 controllers.
About the scan mode, my objective is exactly to support the DMA. I have a local branch doing that already and it would be my next PR. My strategy was to merge it on smaller easier to review pull requests. If you prefer we can close this PR and I can do it all together on a big PR.
Since you use scan mode just for dma, maybe it makes sense to keep the enable and disable scan mode methods.

@rleh rleh assigned chris-durand and unassigned rleh Mar 17, 2023
@chris-durand
Copy link
Member

Since you use scan mode just for dma, maybe it makes sense to keep the enable and disable scan mode methods.

I would just leave it as is and recommend to close the PR without merging. The scan mode does no harm for now. We can finally sort it when you implement ADC DMA in your next PR.

@victorandrehc
Copy link
Contributor Author

victorandrehc commented Mar 17, 2023

The DMA PR is not right yet. But I will create a draft so the progress can be followed.

@victorandrehc victorandrehc deleted the fix/enable_scan_mode_overflow branch February 19, 2024 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
advanced 🤯 ci:hal Triggers the exhaustive HAL compile CI jobs enhancement 🌈
Development

Successfully merging this pull request may close these issues.

4 participants