cpu/cc2538: RTT: implement missing API functions by benpicco · Pull Request #13630 · RIOT-OS/RIOT

benpicco · 2020-03-13T11:06:28Z

Contribution description

The RTT implementation of cpu/cc2538 is based on the sleep timer, a 32 bit 32kHz timer that can only be read and that only supports one alarm. (No overflow interrupt)

This implements functions that are defined by the RIOT API, but are not directly supported by the hardware:

rtt_set_counter(): as we can't set the counter directly, add an offset to the raw counter value instead
rtt_get_alarm(): we can't read back the alarm time, so store it in a local variable. This is also needed for
rtt_set_overflow_cb(): In the previous implementation the overflow callback would always overwrite the alarm callback and vice versa.
To fix this, check which event will occur first and set that alarm as well as a flag to tell which alarm it was.
When the alarm rings, set the alarm to the other event if there is one.

Testing procedure

There are no tests for those functions.

Issues/PRs references

I implemented those for #13519, but then discovered I don't need them.
So I'm not sure if this should be added just for completeness' sake when we could just as well do without.

We can't set the hardware counter directly, so always add an offset.

We can't read back the alarm, so just store it in a variable.

Previously the setting the alarm would overwrite the overflow callback and vice versa. Since we can only set one alarm in hardware, always set the alarm to the closest event of the two.

fjmolinas · 2020-03-31T08:11:24Z

When implementing the driver I had the same questions. I decided to implement a dump interface thinking that the smarted I made rtt.c the more overhead it would have. The documentation already says that timeout shouldn't be set closer that 5cycles from the target time. I though this probably had to do with time taken to read the rtt current value and set an alarm.

If more stuff is happening in the function call then this constraint might be extended. I ended up thinking that this would be something that a timer extension would handle, since setting an overflow alarm and alarm is just the same as setting multiple alarms for this platform.

(I also based my self on another rtt that didn't implement what it didn't support).

I still think this is a good question, but I don't have a strong opinion, maybe @kaspar030 @MichelRottleuthner have a stronger opinion on this?

MichelRottleuthner · 2020-03-31T10:29:02Z

DL;DR, short term solution:
(A): not use this specific timer for the rtt as the missing features break the api
(b): do it as this PR does and provide missing functionality in software

A bit more details:

The documentation already says that timeout shouldn't be set closer that 5cycles from the target time. I though this probably had to do with time taken to read the rtt current value and set an alarm.

Yes, probably related to clock/bus synchronization between the HF and LF clock domains.

If more stuff is happening in the function call then this constraint might be extended.

There a few things to keep in mind here:
-A few cycles of the LF clock are quite long compared to instructions executed on the HF clock so there is at least some room for additional operations.
-the 5 cycles are completely platform dependent and as of now we have no api or define for it. So for now these constraints are practically out of scope of the rtt api and the only safe way out is "know the specific constraints of your target platform". (This should be improved IMO as it is against the usual RIOT philosophy)

I ended up thinking that this would be something that a timer extension would handle, since setting an overflow alarm and alarm is just the same as setting multiple alarms for this platform.

I agree on the point that a timer extension should be in place to handle that.
But a problem with the current design is that the rtt api is not designed to have optional functionality or means for supplementing missing functionality in a generic way.

Long term solution:
There is absolutely no need for a separate rtt api. IMO the rtt api is just an artifact of the fact that we don't have a proper hardware presentation / hardware abstraction architecture.
What we actually need is a common periph_timer api that can cope with things like constraints, optional functionality and software extensions where needed.

kaspar030 · 2020-04-15T10:40:38Z

The documentation already says that timeout shouldn't be set closer that 5cycles from the target time.

I think in this case, may I suggest to just "fake" a timer that actually cannot do this. E.g., if the minimum that should be set is 5, round to something in base 2 (8 or 2**3) and expose the 32khz 32bit timer as 32768>>3 32bit>>3 timer. We shouldn't complicate our lives around crappy hardware.
The software overhead is probably negligible (a couple of extra instructions).

I think it is totally acceptable to do add some missing functionality in software as done in this PR.

fjmolinas · 2020-04-17T08:35:04Z

There is absolutely no need for a separate rtt api. IMO the rtt api is just an artifact of the fact that we don't have a proper hardware presentation / hardware abstraction architecture.
What we actually need is a common periph_timer api that can cope with things like constraints, optional functionality and software extensions where needed.

Is this something you have on your roadmap?

MichelRottleuthner · 2020-04-17T08:52:40Z

Is this something you have on your roadmap?

Absolutely :) As I said a while ago I want to approach this a bit differently, starting from a comprehensive overview of our targeted hardware to put our design decisions on solid grounds. At the moment this is not moving super fast but I'd say pretty thoroughly. Once all the information is polished up for wider discussion it is planned to contribute that to the timer related RDM.

fjmolinas · 2020-04-17T09:02:05Z

@benpicco I think we can go ahead with this one then, is it in a state to be reviewed?

miri64 · 2020-06-25T15:01:25Z

@benpicco I think we can go ahead with this one then, is it in a state to be reviewed?

Ping @benpicco?

benpicco · 2020-06-25T15:06:06Z

It can be reviewed, the question is if we want these features at the cost of the overhead they add to the common case.

fjmolinas

It can be reviewed, the question is if we want these features at the cost of the overhead they add to the common case.

@benpicco I used #14146 to see if there was an impact on RTT_MIN_OFFSET and there was none. So I think the impact is negligible. Regarding the part about implementing functionality that is not in hardware. IMO the best is to have an upper layer that implement in software all the missing hardware functionalities for all platforms, but since there is not such thing yet, lets do it in this platform. So from the mentioned changes:

rtt_get_alarm: we should have this, its easy to implement as you did with a static variable.
rtt_set_overflow_cb() overwrite handling: lets have this since the overflow doesn't seem important.

rtt_set_counter: this one I'm on the fence. There should be at least an empty function declaration, not having it was an error on my side. Regarding the functionality itself nrf52 doesn't have the functionality and chose not to implement. Below is a patch that removes only this part from your PR. I'll let you as the contributor decide on this one.

git diff cpu

diff --git a/cpu/cc2538/periph/rtt.c b/cpu/cc2538/periph/rtt.c
index 58ca39cdbb..a0b7c05b33 100644
--- a/cpu/cc2538/periph/rtt.c
+++ b/cpu/cc2538/periph/rtt.c
@@ -36,7 +36,6 @@ static rtt_cb_t overflow_cb = NULL;
static void *overflow_arg;

static uint32_t rtt_alarm;
-static uint32_t rtt_offset;

static enum {
    RTT_ALARM,
@@ -68,10 +67,11 @@ void rtt_poweroff(void)

void rtt_init(void)
{
+    rtt_alarm = 0;
    rtt_poweron();
}

-static inline uint32_t _rtt_get_counter(void)
+uint32_t rtt_get_counter(void)
:...skipping...
diff --git a/cpu/cc2538/periph/rtt.c b/cpu/cc2538/periph/rtt.c
index 58ca39cdbb..a0b7c05b33 100644
--- a/cpu/cc2538/periph/rtt.c
+++ b/cpu/cc2538/periph/rtt.c
@@ -36,7 +36,6 @@ static rtt_cb_t overflow_cb = NULL;
static void *overflow_arg;

static uint32_t rtt_alarm;
-static uint32_t rtt_offset;

static enum {
    RTT_ALARM,
@@ -68,10 +67,11 @@ void rtt_poweroff(void)

void rtt_init(void)
{
+    rtt_alarm = 0;
    rtt_poweron();
}

-static inline uint32_t _rtt_get_counter(void)
+uint32_t rtt_get_counter(void)
{
    return ((SMWDTHROSC_ST0 & 0xFF)
        | ((SMWDTHROSC_ST1 & 0xFF) << 8)
@@ -79,21 +79,9 @@ static inline uint32_t _rtt_get_counter(void)
        | ((SMWDTHROSC_ST3 & 0xFF) << 24));
}

-uint32_t rtt_get_counter(void)
-{
-    return _rtt_get_counter() - rtt_offset;
-}
-
void rtt_set_counter(uint32_t counter)
{
-    rtt_alarm -= rtt_offset;
-    rtt_offset = _rtt_get_counter() + counter;
-    rtt_alarm += rtt_offset;
-
-    /* re-set the overflow callback */
-    if (overflow_cb) {
-        rtt_set_overflow_cb(overflow_cb, overflow_arg);
-    }
+    (void) counter;
}

static void _set_alarm(uint32_t alarm)
@@ -110,19 +98,19 @@ void rtt_set_alarm(uint32_t alarm, rtt_cb_t cb, void *arg)
    assert(cb && !(alarm & ~RTT_MAX_VALUE));

    unsigned irq = irq_disable();
-    uint32_t now = _rtt_get_counter();
+    uint32_t now = rtt_get_counter();

-    rtt_alarm = alarm + rtt_offset;
+    rtt_alarm = alarm;

    /* set callback*/
    alarm_cb = cb;
    alarm_arg = arg;

-    DEBUG("rtt_set_alarm(%lu), alarm in %lu ticks, overflow in %lu ticks\n",
-          alarm, rtt_alarm - now, rtt_offset - now);
+    DEBUG("rtt_set_alarm(%lu), now %lu ticks, overflow in %lu ticks\n",
+          alarm, now, RTT_MAX_VALUE - now);

    /* only set overflow alarm if it happens before the scheduled alarm */
-    if (overflow_cb == NULL || (rtt_offset - now >= rtt_alarm - now)) {
+    if (overflow_cb == NULL || (rtt_alarm >= now)) {
        rtt_next_alarm = RTT_ALARM;
        _set_alarm(rtt_alarm);
    }
@@ -132,7 +120,7 @@ void rtt_set_alarm(uint32_t alarm, rtt_cb_t cb, void *arg)

uint32_t rtt_get_alarm(void)
{
-    return rtt_alarm - rtt_offset;
+    return rtt_alarm;
}

void rtt_clear_alarm(void)
@@ -147,16 +135,16 @@ void rtt_clear_alarm(void)
void rtt_set_overflow_cb(rtt_cb_t cb, void *arg)
{
    unsigned irq = irq_disable();
-    uint32_t now = _rtt_get_counter();
+    uint32_t now = rtt_get_counter();

    /* set callback*/
    overflow_cb = cb;
    overflow_arg = arg;

    /* only set overflow alarm if it happens before the scheduled alarm */
-    if (alarm_cb == NULL || (rtt_alarm - now > rtt_offset - now)) {
+    if (alarm_cb == NULL || (rtt_alarm <= now)) {
        rtt_next_alarm = RTT_OVERFLOW;
-        _set_alarm(rtt_offset);
+        _set_alarm(RTT_MAX_VALUE);
    }

    irq_restore(irq);
@@ -174,7 +162,7 @@ void rtt_clear_overflow_cb(void)
void isr_sleepmode(void)
{
    rtt_cb_t tmp;
-    bool both = (rtt_alarm == rtt_offset);
+    bool both = (rtt_alarm == RTT_MAX_VALUE);

    switch (rtt_next_alarm) {
    case RTT_ALARM:
@@ -194,18 +182,18 @@ void isr_sleepmode(void)
        break;
    }

-    uint32_t now = _rtt_get_counter();
+    uint32_t now = rtt_get_counter();

-    if (alarm_cb && (rtt_offset - now >= rtt_alarm - now)) {
+    if (alarm_cb && (rtt_alarm >= now)) {
        DEBUG("rtt: next alarm in %lu ticks (RTT)\n", rtt_alarm - now);

        rtt_next_alarm = RTT_ALARM;
        _set_alarm(rtt_alarm);
-    } else if (overflow_cb && (rtt_alarm - now > rtt_offset - now)) {
-        DEBUG("rtt: next alarm in %lu ticks (OVERFLOW)\n", rtt_offset - now);
+    } else if (overflow_cb && (rtt_alarm < now)) {
+        DEBUG("rtt: next alarm in %lu ticks (OVERFLOW)\n", RTT_MAX_VALUE - now);

        rtt_next_alarm = RTT_OVERFLOW;
-        _set_alarm(rtt_offset);
+        _set_alarm(RTT_MAX_VALUE);
    }

    cortexm_isr_end();

fjmolinas · 2020-07-23T07:53:59Z

cpu/cc2538/periph/rtt.c

+
+        rtt_next_alarm = RTT_ALARM;
+        _set_alarm(rtt_alarm);
+    } else if (overflow_cb && (rtt_alarm - now > rtt_offset - now)) {


Suggested change

} else if (overflow_cb && (rtt_alarm - now > rtt_offset - now)) {

} else if (overflow_cb && (rtt_alarm > rtt_offset)) {

fjmolinas · 2020-07-23T07:54:08Z

cpu/cc2538/periph/rtt.c

+
+    uint32_t now = _rtt_get_counter();
+
+    if (alarm_cb && (rtt_offset - now >= rtt_alarm - now)) {


Suggested change

if (alarm_cb && (rtt_offset - now >= rtt_alarm - now)) {

if (alarm_cb && (rtt_offse >= rtt_alarm)) {

fjmolinas · 2020-07-23T07:54:23Z

cpu/cc2538/periph/rtt.c

    overflow_arg = arg;

+    /* only set overflow alarm if it happens before the scheduled alarm */
+    if (alarm_cb == NULL || (rtt_alarm - now > rtt_offset - now)) {


Suggested change

if (alarm_cb == NULL || (rtt_alarm - now > rtt_offset - now)) {

if (alarm_cb == NULL || (rtt_alarm > rtt_offset )) {

fjmolinas · 2020-07-23T07:54:32Z

cpu/cc2538/periph/rtt.c

+          alarm, rtt_alarm - now, rtt_offset - now);
+
+    /* only set overflow alarm if it happens before the scheduled alarm */
+    if (overflow_cb == NULL || (rtt_offset - now >= rtt_alarm - now)) {


Suggested change

if (overflow_cb == NULL || (rtt_offset - now >= rtt_alarm - now)) {

if (overflow_cb == NULL || (rtt_offset >= rtt_alarm )) {

I wonder if this was to handle overflows, but can't remember.

miri64 · 2020-08-04T17:18:20Z

Taken over (hopefully in agreement with @benpicco, the conversation at hand does not provide that info) by @fjmolinas in #14700.

fjmolinas · 2020-08-05T07:16:38Z

Taken over (hopefully in agreement with @benpicco, the conversation at hand does not provide that info) by @fjmolinas in #14700.

Sorry yes, @benpicco did not have time/interest to maintain the PR, he agreed with me doing so.

benpicco added 3 commits March 13, 2020 11:23

cpu/cc2538: rtt: implement rtt_set_counter()

3661570

We can't set the hardware counter directly, so always add an offset.

cpu/cc2538: rtt: implement rtt_get_alarm()

3097370

We can't read back the alarm, so just store it in a variable.

cpu/cc2538: rtt: allow to set alarm and overflow cb independently

ce383fe

Previously the setting the alarm would overwrite the overflow callback and vice versa. Since we can only set one alarm in hardware, always set the alarm to the closest event of the two.

benpicco added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: cpu Area: CPU/MCU ports State: demonstrator State: This PR (loosely) demonstrates a concept and is not necessarily meant to be merged. labels Mar 13, 2020

benpicco requested a review from MrKevinWeiss as a code owner March 13, 2020 11:06

benpicco requested review from fjmolinas and haukepetersen March 13, 2020 11:07

benpicco mentioned this pull request Mar 13, 2020

drivers/rtt_rtc: add RTT based RTC implementation, enable it for cpu/cc2538, nrf5x_common #13519

Merged

fjmolinas reviewed Jul 23, 2020

View reviewed changes

fjmolinas mentioned this pull request Aug 4, 2020

cpu/cc2538: RTT: implement missing API functions #14700

Merged

benpicco closed this Aug 4, 2020

benpicco deleted the cc2538_enhance_rtt branch August 4, 2020 17:14

benpicco mentioned this pull request Sep 20, 2020

doc/memos: RDM on a unified low-level timer-API #15046

Closed

	} else if (overflow_cb && (rtt_alarm - now > rtt_offset - now)) {
	} else if (overflow_cb && (rtt_alarm > rtt_offset)) {


		uint32_t now = _rtt_get_counter();

		if (alarm_cb && (rtt_offset - now >= rtt_alarm - now)) {

	if (alarm_cb && (rtt_offset - now >= rtt_alarm - now)) {
	if (alarm_cb && (rtt_offse >= rtt_alarm)) {

	if (alarm_cb == NULL \|\| (rtt_alarm - now > rtt_offset - now)) {
	if (alarm_cb == NULL \|\| (rtt_alarm > rtt_offset )) {

	if (overflow_cb == NULL \|\| (rtt_offset - now >= rtt_alarm - now)) {
	if (overflow_cb == NULL \|\| (rtt_offset >= rtt_alarm )) {

Conversation

benpicco commented Mar 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

fjmolinas commented Mar 31, 2020

Uh oh!

MichelRottleuthner commented Mar 31, 2020

Uh oh!

kaspar030 commented Apr 15, 2020

Uh oh!

fjmolinas commented Apr 17, 2020

Uh oh!

MichelRottleuthner commented Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjmolinas commented Apr 17, 2020

Uh oh!

miri64 commented Jun 25, 2020

Uh oh!

benpicco commented Jun 25, 2020

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

fjmolinas Jul 23, 2020

Choose a reason for hiding this comment

Uh oh!

fjmolinas Jul 23, 2020

Choose a reason for hiding this comment

Uh oh!

fjmolinas Jul 23, 2020

Choose a reason for hiding this comment

Uh oh!

fjmolinas Jul 23, 2020

Choose a reason for hiding this comment

Uh oh!

benpicco Aug 7, 2020

Choose a reason for hiding this comment

Uh oh!

miri64 commented Aug 4, 2020

Uh oh!

fjmolinas commented Aug 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

benpicco commented Mar 13, 2020 •

edited

Loading

MichelRottleuthner commented Apr 17, 2020 •

edited

Loading