RP time driver alternatively using mtimer, timer0, timer1, or aot

[mschnell1]

adding potential features in cargo.toml

[bugadani]

Why are you closing a PR, and reopening a new one with the same thing?

[mschnell1]

Why are you closing a PR, and reopening a new one with the same thing?

I am not familiar with GitHub PRs (Just plain old GitLab.)

I did not think a PR that issued error message to me is of any relevance and therefore I closed them.

Is the better way to "elongate" the PR by committing something to the fork and then do (what ?) ?

Anyway. Even though it now actually seems to build in the pipeline I was not able to have the commit be accepted to be error-free, and I don't know what I could improve (in fact I even needed to mingle the cargo.toml file inappropriately to make it build).

I suppose the problem is due to the cfg features settings I introduced.

Here I am not sure if I did it correctly.

IMHO for the 235x the default should be the mtimer.

• The users will prefer to use the more versatile timer0 and timer1 for more complex purposes.
• mtimer might be more secure due to alarm comparing greater instead of equal and extremely simple interrupt handling (just statically controlled by the alarm value).

I think,. the system tick should be somehow automatically be 1_000_000 with timer0, timer1 and mtimer, but 1_000 with aot.
I have no idea how to accomplish this.

BTW: I can't compile the cloned repository due to error: failed to install component: 'llvm-tools-preview-x86_64-pc-windows-msvc', detected conflict: 'lib/rustlib/x86_64-pc-windows-msvc/bin/llvm-ar.exe'

[bugadani]

If you commit to the same branch, the PR will be updated, and CI will run on it. However, closing and creating a new PR will send 2 messages to the matrix chat for 1700 people to see.

[mschnell1]

If you commit to the same branch, the PR will be updated, and CI will run on it. However, closing and creating a new PR will send 2 messages to the matrix chat for 1700 people to see.

Ooops :(
Even if the pipeline fails ? (I supposed exactly nothing to happen in that case)
Weird ....

So just committing tho the forked is enough to have you be aware of the changes (even if the pipeline fails) ?

[mschnell1]

Should we refacture this moving the code for each hardware alternative to a dedicated module ?
(Right now I tried to touch the original code as little as possible.)

[Dirbaio]

Could you fix CI?

• To fix rustfmt, just do cargo +nightly fmt.
• To fix the "test" job, make sure cfg's are setup to never use any #[interrupt] if the rt feature is off.

[Dirbaio]

Could you add documentation for the features? Use ## just like the existing comments.

[mschnell1]

I did the modification in the toml file not as a suggestion but just to allow the pipeline to do the build.
Supposedly, in the end these features should not be allowed in the project, but in the embassy-time dependency section. But I have no idea how this should be done.

[Dirbaio]

The Cargo features would make more sense if we did the following changes

• Remove the time-driver feature. Instead, the time driver is automatically enabled if any of the time-driver-* features is enabled.
  • To do this you could rename the time-driver feature to _time-driver so that it's "private", and make the time-driver-* features enable it.
• Add a time-driver-timer0 feature instead of making it the default if no other time driver is enabled.

To make examples compile you'll have to edit their Cargo.toml.
Also, please add to ci.sh lines that test compiling embassy-rp with the 4 different time drivers, so CI checks they all compile.

[mschnell1]

OK. As I am not at all experienced in doing cargo instructions, I think it would be good if you could do the decisions and the (suggestions for) implementation.

My idea was as such:

• if there is "integrated-timers" in the embassy-executor section, the time driver is used.
• if there are no additional instructions in the embassy-time section :
  • if its a RP2040, the #[cfgs use the "TIMER" (already in place)
  • if its a RP235x, the #[cfgs use the RISC V System timer in pac::SIO (currently in place: TIMER0.)
• if there is a feature "time-driver-timer0", "time-driver-timer1", or "time-driver-aot" in the embassy-time section, the #[cfgs use the appropriate timer: TIMER0, TIMER1, or the timer in pac::POWMAN.
• if the feature "time-driver-aot" is given in the embassy-time section, the feature "tick-hz-1_000" is automatically set to prohibit any ambiguity (while otherwise the feature "tick-hz-1_000_000" is set (as it seemingly is right now, as trying a different setting when "integrated-timers" is enabled fails to compile.

Can such be implemented ?
Or is some other implementation (with no "default" setting) more appropriate ?

I suggest the RISK V system timer as a default, as

• it's provided exactly for this "system" purpose by the hardware design.
• the user might want to use the more versatile timers "0" and "1" for dedicated purposes
• it counts 64 bits, the alarm checks all 64 bits. Hence no 32 Bit / 72 hours considerations necessary.
• I feel the RISK V system timer is slightly more "secure" as the interrupt simply is managed by the alarm register content and stays active until same is lower than the count register. No additional conditions.
• The RISK V system timer is in the "Secure SIO" . This might be advantageous - and works with the examples, but it also might be a problem in certain situations ?!?!?!

BTW: the tick generator is set up for driving TIMER0 with 1_000_000 Hz in embassy-rp-0.3.1/src/clocks.rs:529 ff. This seems not appropriate when TIMER0 is not used. Maybe it makes sense just to set just the constants appropriately so that clocks::clk_ref_freq() can be used in the timer driver to configure the appropriate clock to drive the timer to be enabled (as shown in the suggested driver code ).