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

adc delays for seconds on init #206

Closed
David-OConnor opened this issue Mar 6, 2021 · 9 comments · Fixed by #217
Closed

adc delays for seconds on init #206

David-OConnor opened this issue Mar 6, 2021 · 9 comments · Fixed by #217
Assignees
Labels
bug Something isn't working

Comments

@David-OConnor
Copy link
Contributor

David-OConnor commented Mar 6, 2021

This is run after calibrate:

adc.rs:

// ADEN bit cannot be set during ADCAL=1
// and 4 ADC clock cycle after the ADCAL
// bit is cleared by hardware

// ...

fn wait_adc_clk_cycles(&self, cycles: u32) {
    let adc_clk_cycle = self.clocks.hclk().0 / (self.ckmode as u32);
    asm::delay(adc_clk_cycle * cycles);
}

I'm not sure what an ADC clock cycle is, but don't think it's right here. For example, the first variable will be on the order of 10^6. I think maybe it should be asm::delay(self.ckmode as u32 * cycles); And, remove this fn and move the delay to right after the cal, since it's now a one-line one-off.

This causes consequences like long boot-up time, and disconnecting USB devices (or anything that requires a quick response and is initialized before the ADC)

@Sh3Rm4n Sh3Rm4n added the bug Something isn't working label Mar 7, 2021
@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Mar 7, 2021

Currently, I don't really see the concrete problem / best solution (have'nt looked deeply into it), but this is clearly a problem of being a blocking function.

I think this function should not be called implicitly inside the code and should be visible to the user. At best, this function could just calculate an estimate time, which is needed, so that the ADC is guaranteet to be ready (calibration is done) and the user can choose the way to go about it (blocking himself via asm::delay or some timer, or going the async / dispension point route)

@David-OConnor
Copy link
Contributor Author

Is the original code consistent with the RM note about cycles? Ie is a multi-second delay what's specified, or only a multi-tick delay?

@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Mar 7, 2021

I don't know. I would have to look into the RM myself, to find out.

EDIT: When I reviewed the code, I assumed that it is specified in the RM, but this might also be an oversight.

@David-OConnor
Copy link
Contributor Author

The main part is what you put in the comment. I'm just not sure how to interpret it. Leaning towards the modification I posted here.

@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Mar 8, 2021

Well in your suggestions the delay is dependent on the system clock, which is in most cases different from the clock, which the adc peripheral uses. So this could lead to problems or oversights. Nonetheless, the current implementation has non-desirable behavior.

I don't know if the delay calculation is wrong and does lead to too long delays, or if we need a non-blocking way.

@David-OConnor
Copy link
Contributor Author

I give up

@Sh3Rm4n Sh3Rm4n self-assigned this Mar 28, 2021
@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Mar 28, 2021

I'm sorry that you are unhappy with the responsiveness.

But I can't help you but say that this is an open source project managed by volunteers which devote their free time into this project.

Currently I'm the only (not so) frquent maintainer of this repository.
In the last months I was happy to devote 1-3 hours a week to this project. I haven't had the capacity to do more. #111 is still relevant.

Thank you for all your valuable reports about issues and bugs of this project. These are important to get an idea, what should be fixed to improve the quality of the code.

However I can't emphasize enough, that this is a voluntary project. If you want to see this project progressing, help us. Make the work of the maintainer as easily as possible. Make a good quality contribution where the only thing I've to do is to clock the merge button.

But even than I can't guarantee you that'll happen immediately.
I'll try my best to keep up and make frequent small maintenance work like looking through issues and responding to PRs.
But reviewing PRs and doing bug analysis do take a lot of time, which I don't have most of the time.

And last of all: I still want to have fun working in this project. If it feels like work I have to do, I'm not motivated because I already have a job.

So sorry for the long rant. I hope you understand these points. I find your contribution valuable but you should be patient when working with volunteers :)

@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Mar 28, 2021

On a technical side, this issue is still relevant, so I'll reopen it.

@Sh3Rm4n Sh3Rm4n reopened this Mar 28, 2021
@David-OConnor
Copy link
Contributor Author

David-OConnor commented Mar 28, 2021

Thanks for the work. I get it - I have an open source proj in a similar position.

I'm developing several commercial products using stm32, and wanted to contribute to the ecosystem as I go. This and related libraries are positioned to be the foundation of a thriving ecosystem. An increasingly digital world where hardware is cheap; a popular, varied, modern line of MCUs; a beautiful language, and enough community momentum to try it. If you ever decide to give this another shot, I think you'll find (maybe not immediately, but eventually) it could be the foundation of many cool projects and products.

Related: There's more similarity than differences across the stm32 families - it may be worth collaborating with the other stm32fyxx-hal maintainers.

Rahix added a commit to Rahix/stm32f3xx-hal that referenced this issue Apr 25, 2021
As explained in issue stm32-rs#206, the code here is doing the calculation
completely wrong.  This leads to a multi-second startup delay instead of
just a few clock cycles.

Fix this by using the correct calculation; currently only ADC clock
configurations synchronous to the AHB are supported which makes this
quite easy: We just need to delay the given number of cycles * the
selected ADC clock prescaler (1, 2, or 4).

To make this code future proof against an implementation of asynchronous
ADC clock configurations, use a `match` statement here which will fail
to compile once the `ASYNCHRONOUS` variant is added to the `CkMode`
struct - thus forcing the implementor to take another look at this.

Fixes: stm32-rs#206
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants