Repository navigation
Conversation
github-actions
Bot
added
Area: boards
AnnsAnns
added
the
CI: ready for build
crasbe
added
the
Type: new feature
| /** | ||
| * @ingroup cpu_rp2350 | ||
| * @{ | ||
| * |
There was a problem hiding this comment.
Maybe you can add a comment here which date and version the RP2350 Reference Manual has that is referenced here. That will make it much easier in 10 years when all the references have changed 😅
There was a problem hiding this comment.
where is the fun in that, future generations need to be scared of me in the git blame
| if (!timeout) { | ||
| /* catch case that a tick happens right before arming the alarm */ |
There was a problem hiding this comment.
I find this confusion. Firstly I'd write if(timeout == 0), as it is an integer and not a boolean. Same comparison, but the former is IMO clearer than the latter.
Secondly, the comment does not really fit the code, does it? The alarm won't be armed for timeout == 0, instead the callback is just called once.
There was a problem hiding this comment.
I have to be honest I forgot the context of this
| return 0; | ||
| } | ||
|
|
||
| int timer_set_absolute(tim_t dev, int channel, unsigned int value) |
There was a problem hiding this comment.
Does this function need a check for value >= 0?
Does a value of 0 or a negative value make sense?
There was a problem hiding this comment.
I'm not following, isn't that the idea of making it unsigned?
There was a problem hiding this comment.
I probably overlooked that it is unsigned.
Still, is a value of 0 permissible?
| return 0; | ||
| } | ||
|
|
||
| int timer_set_periodic(tim_t dev, int channel, unsigned int value, uint8_t flags) |
There was a problem hiding this comment.
Does this function need a check for value >= 0?
Does a value of 0 or a negative value make sense?
| unsigned state = irq_disable(); | ||
| /* The ARMED bits are write-1-clear (table 1236) */ | ||
| DEV(dev)->ARMED = 1u << channel; | ||
| flag_periodic[dev] &= ~(1u << channel); |
There was a problem hiding this comment.
Should we also reset the _flag_reset flag here? Not that it would matter much (it is set and reset by timer_set), but maybe good practice?
|
Haven't tested it myself yet and I really don't have the time to at the moment, I just did some minor procrastination here. |
|
Thank you for looking into this, your procrastination counters mine, it's Yin and Yang |
crasbe
left a comment
crasbe
left a comment
There was a problem hiding this comment.
Why not use the existing register defines?
I'm currently looking into getting the RP2350.h to be more compatible in general in the hopes of removing all defines that exist in the sdk, there is some weirdness still |
I don't quite follow? |
AnnsAnns
left a comment
AnnsAnns
left a comment
There was a problem hiding this comment.
ty for the reviews, since I have an exam on Monday my brain decided that now is the perfect time to look at this instead of doing stuff for my exam 🤠👍
| return -ENODEV; | ||
| } | ||
| /* TICKS blocks are designed to generate 1 us ticks, so we can only support that frequency */ | ||
| assert(freq == US_PER_SEC); |
There was a problem hiding this comment.
I forgot the complete context of this but as far as I remember that frequency is used internally for timers yes
| if (!timeout) { | ||
| /* catch case that a tick happens right before arming the alarm */ |
There was a problem hiding this comment.
I have to be honest I forgot the context of this
| return 0; | ||
| } | ||
|
|
||
| int timer_set_absolute(tim_t dev, int channel, unsigned int value) |
There was a problem hiding this comment.
I'm not following, isn't that the idea of making it unsigned?
| return 0; | ||
| } | ||
|
|
||
| int timer_set_periodic(tim_t dev, int channel, unsigned int value, uint8_t flags) |
|
oh wow my comments went wrong af |
AnnsAnns
added
the
AI: Co-Authored
|
Wanted to try this, but it isn't compiling for me: Compile log |
|
ah yeah that came with the reviews I'll look into it, haven't compiled post review yet, ty for pointing that out |
crasbe
added
the
State: waiting for author
|
Fixed the compilation error, should work again. Can't retest it because I accidentally took a pico without any pins with me .... and I also accidentally took a pico 1 and not a pico 2 with me 😢 But the underlying timer stuff shouldn't have changed so I'd be surprised if it stopped working |
AnnsAnns
removed
the
State: waiting for author
|
It seems like the timer only works immediately after flashing and not after a reset. This is Also the time is kinda wrong, the two seconds take four seconds and the five seconds take ten. |
|
Unsurprisingly, the |
crasbe
added
the
State: waiting for author
|
Will look into this this week (hopefully) since this reconfirms my suspicion that I messed something up with the frequencies in my port |
Appears to be correct now, in the process I realized that I added a lot of noise to the riscv irq when porting the pico2 |
d685825 to
753c314
Compare
|
|
||
| if (timeout == 0) { | ||
| if (ctx_cb[dev]) { | ||
| ctx_cb[dev](ctx_arg[dev], channel); |
There was a problem hiding this comment.
This was copied 1:1 from the pico 1 implementation, why is this wrong?
There was a problem hiding this comment.
From IRL conversation:
He is ragebaiting me 🥀 The relevant line in timer.h that explains what it was supposed to do was added in his PR 3 weeks ago, like other timers the pico1 did it incorrectly
| #include "regs/resets.h" | ||
|
|
||
| /** Number of alarm channels per timer block */ | ||
| #define TIMER_CHANNEL_NUMOF 4u |
There was a problem hiding this comment.
Added the isr macros the the pico1 also has (though I don't see them being used anywhere). I don't see any other defines required in the timer.h / pico1, either they are somewhere else where I'm not looking or I'm defining everything the pico1 is
That output looks very wrong |
from more like mildly wrong main seem to be restarting and has no message when it sets a timer I suggest adding a message at the beginning of |
What karl said but I agree that without context the prints look confusing |
Updated according to new timer.h comments |
|
also works
|
|
This needs a rebase after the |
|
Perhaps you can wait a bit longer, I have a PR coming up that fixes the incorrect |
Oops, my github was still on the previous comment 🙈 |
Co-authored-by: crasbe <crasbe@gmail.com>
Contribution description
Part of the ongoing "wait people actually use the pico2, oh god why" series. This implements timer support, hooray. Timers are surprisingly not that complicated, I just never looked at them when adding support because I didn't have a proper use outside of xosc_sleep which was enough for my needs.
Testing procedure
cd examples/guides/timerssudo BOARD=rpi-pico-2-arm make flash term PORT=/dev/ttyUSB0Issues/PRs references
Declaration of AI-Tools / LLMs usage:
AI-Tools / LLMs that were used are:
-The initial timer was based on the pico 1 timer together with Claude Fable 5, mostly to experiment with it, I cross checked its claims with the datasheet at all times, chapter 12.8 and related timer tables are fairly understandable.