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

Assorted Delay feedback #7

Closed
martindisch opened this issue Mar 1, 2020 · 2 comments
Closed

Assorted Delay feedback #7

martindisch opened this issue Mar 1, 2020 · 2 comments

Comments

@martindisch
Copy link
Contributor

Besides the overflow MR, I also have some less concrete feedback:

At first when I tried the code sample you provided in martindisch/gd32vf103-demo#1, I ran into an error

error[E0432]: unresolved import `gd32vf103_hal::rcu::Strict`
  --> src/main.rs:11:55
   |
11 |     ctimer::CoreTimer, delay::Delay, pac, prelude::*, rcu::Strict,
   |                                                       ^^^^^^^^^^^ no `Strict` in `rcu`

But I quickly noticed that this is just because I was using the 0.0.2 version from crates.io which doesn't work with that yet (even though it's pretty recent). Using the latest from Git solved it for me. This is not really an issue, as it will be fixed when you publish the next version of your crate.

Another thing concerns the delay accuracy. Just like you, I don't have any equipment I can use to do exact measurements, but small inaccuracies aren't the issue here. On my device it seems more like it's off by a factor of 2. I adjusted the delay blinky example to use a 10000 ms delay and when timing by hand, the LED only ended up being toggled about every 20 seconds. I didn't read any documentation so am not confident enough to go ahead and issue a MR, but it seems to me like maybe the divide by 2 in delay.rs is wrong and should be 4 instead?

diff --git a/src/delay.rs b/src/delay.rs
index ad3af92..ccf1b52 100644
--- a/src/delay.rs
+++ b/src/delay.rs
@@ -32,7 +32,7 @@ impl<T: Into<u32>> DelayMs<T> for Delay {
     // according to the clock diagram, but 2 is accurate. I suspect
     // this will need to change with further documentation updates.
     fn delay_ms(&mut self, ms: T) {
-        let count: u32 = ms.into() * (self.clock_frequency.0 / 1000 / 2);
+        let count: u32 = ms.into() * (self.clock_frequency.0 / 1000 / 4);
         // todo: avoid using u64 values in this function
         let tmp: u64 = self.ctimer.get_value();
         let end = tmp + count as u64;

At least on my device this fixed the problem and now I'm very close to the 10 seconds I set the delay for.

@martindisch martindisch changed the title Assorted feedback Assorted Delay feedback Mar 1, 2020
@martindisch martindisch changed the title Assorted Delay feedback Assorted Delay feedback Mar 1, 2020
@luojia65
Copy link
Member

luojia65 commented Mar 1, 2020

Thanks for your issue! Unlike other peripheral, the CoreTimer isn't documented in the chip's User Manual, but it's on chip ip-core's documents without any factor given. I'll go and accept divide by 4 for now at 2bef5b3. (I may need further confirmation for the factor and which clock it actually uses, or get a mesurement device to find this out.)
I'm working for serial module, after a prototype is done, I'll release 0.0.3 version of this hal. Thanks!

@martindisch
Copy link
Contributor Author

Great, thanks for your excellent work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants