Porting the Arduino millis(), possible? Topic is solved

[aclassifier]

Good question, since yours compiled!

Here I have the problem. I have four files, really, but I mimicked the structure here. I have moved millis_state_t my_millis = {0,0}; from inside one of the tasks to above main.

In my head your code should have caused the same violation.. Because the violation is correctly reported (in my head).

Code: Select all

// See http://www.xcore.com/viewtopic.php?f=26&t=6470&start=10

#include <stdio.h>
#include <xs1.h>
#include <iso646.h>
#include <timer.h>     // delay_milliseconds(200), XS1_TIMER_HZ etc

typedef enum {false,true} bool;

#define DEBUG_PRINT_TEST 1
#define debug_print(fmt, ...) do { if(DEBUG_PRINT_TEST) printf(fmt, __VA_ARGS__); } while (0)

// ---- Would be in a header file ----

typedef struct millis_state_t {
    unsigned long tick_hi;
    unsigned long last_tick;
} millis_state_t;

millis_state_t millis_state; // "extern" or not makes no difference here, but I do need it in the header file

#define MILLIS() millis_p(millis_state)

// ---- Would be in a source file ----

unsigned long millis_p (millis_state_t &millis_state) {
    timer tmr;
    unsigned long long total_ticks;
    unsigned long current_tick;
    tmr :> current_tick;
    if( current_tick < millis_state.last_tick ) {
        ++millis_state.tick_hi;
    }
    millis_state.last_tick = current_tick;
    total_ticks = ((unsigned long long)millis_state.tick_hi << 32) | (unsigned long long)current_tick;
    return (unsigned long)(total_ticks / (unsigned long long)XS1_TIMER_KHZ);
}

// ---- I actually have two files for two different tasks ----

unsigned digitalRead (void) {
    delay_milliseconds (200);
    return (0);
}

#define RF69_TX_LIMIT_MS 1000
#define TEST_TYPE unsigned long

void test_task (int task_num) {

    TEST_TYPE millis_;
    TEST_TYPE txStart;
    unsigned  testCnt = 0;
    bool      not_timed_out;

    while (testCnt < 2000) {
        txStart = MILLIS();
        do {
            millis_ = MILLIS();
            not_timed_out = (millis_ - txStart) < RF69_TX_LIMIT_MS;
            debug_print ("task %d: testCnt(%d), millis(%d), txStart(%d), millis_-txStart(%d), timedOut(%d)\n",
                    task_num, testCnt, millis_, txStart, millis_ - txStart, !not_timed_out);
            delay_milliseconds(20);
        } while ((digitalRead() == 0) && not_timed_out);
        testCnt++;
    }
}

// ---- In main.xc ----

millis_state_t my_millis = {0,0};

int main() {
    par { // error: use of `millis_state' violates parallel usage rules
        test_task(0);
        test_task(1);
    }
    return 0;
}

Here is the error log (line numbering don't match, I use a long file with #ifdefs for all kinds of code testing):

Code: Select all

11:20:33 **** Incremental Build of configuration Default for project _xmos_issues ****
xmake CONFIG=Default all 
Checking build modules
No build modules used.
Analyzing main.xc
Creating dependencies for main.xc
Compiling main.xc
Creating xmos_issues_nnnm.xe
../src/main.xc:1701:9: error: use of `millis_state' violates parallel usage rules
        par { // error: use of `millis_state' violates parallel usage rules
        ^~~
../src/main.xc:1684:23: note: object used here
            txStart = MILLIS();
                      ^~~~~~~~
../src/main.xc:1649:31: note: expanded from here
    #define MILLIS() millis_p(millis_state)
                              ^~~~~~~~~~~~
../src/main.xc:1684:23: note: object used here
            txStart = MILLIS();
                      ^~~~~~~~
../src/main.xc:1649:31: note: expanded from here
    #define MILLIS() millis_p(millis_state)
                              ^~~~~~~~~~~~
xmake[1]: *** [bin//xmos_issues_nnnm.xe] Error 1
xmake: *** [bin//xmos_issues_nnnm.xe] Error 2

11:20:34 Build Finished (took 863ms)

I like this. We're onto something, aren't we?

Update: It's the same task that's instantiated twice. And when it contains the global instatiation it incorrectly (?) does not report the violation?

[akp]

Each core that uses millis_p must have its own declaration of millis_state to so it doesn't have simultaneous access problem. That's why I declared it inside the test task. I concur that it's inconvenient to do it that way, since it doesn't really give you what you want. I just am not convinced that millis_g is safe without locks on the global variables, maybe you can figure out a way to put that update in a critical section or lock it. You might want to make a combination of c and xc implementation in order to use lib_locks properly, I don't know.

[aclassifier]

Ok, I see what you mean. That violation error means that we have to put millis_state inside each task (core is not strong enough(?)) that uses this kind of timeout mechanism. And the function millis_p is reentrant and thread safe without side effects. However, I would make its parameter explicit, instead of the local millis_state being picked up by the macro. You probably explicitly wanted that parameter to be invisible (like I also do some times), but from that followed that one (I at least, so there is at least one of us) was confused by where in the world should state be declared. So I wasn't onto anything at all with respect to parallel usage analysis. It was correct. One explicit parameter gives "more" code but helps (my) understanding. So I suggest this code, still courtesy akp (I have removed the file-related debris from the last example). Search for "NEW". There is no violation below. Thanks akp!

Code: Select all

#include <xs1.h>
#include <stdio.h>
#include <iso646.h>
#include <timer.h> // delay_milliseconds(200), XS1_TIMER_HZ etc

typedef enum {false,true} bool;

#define DEBUG_PRINT_TEST 1
#define debug_print(fmt, ...) do { if(DEBUG_PRINT_TEST) printf(fmt, __VA_ARGS__); } while (0)

typedef struct millis_state_t {
    unsigned long tick_hi;
    unsigned long last_tick;
} millis_state_t;

#define MILLIS(s) millis_p(s) // NEW with parameter in and out

unsigned long millis_p (millis_state_t &millis_state) {
    timer tmr;
    unsigned long long total_ticks;
    unsigned long current_tick;
    tmr :> current_tick;
    if( current_tick < millis_state.last_tick ) {
        ++millis_state.tick_hi;
    }
    millis_state.last_tick = current_tick;
    total_ticks = ((unsigned long long)millis_state.tick_hi << 32) | (unsigned long long)current_tick;
    return (unsigned long)(total_ticks / (unsigned long long)XS1_TIMER_KHZ);
}

unsigned digitalRead (void) {
    delay_milliseconds (200);
    return (0);
}

#define RF69_TX_LIMIT_MS 1000
#define TEST_TYPE unsigned long

void test_task (int task_num) {

    TEST_TYPE      millis_;
    TEST_TYPE      txStart;
    unsigned       testCnt = 0;
    bool           not_timed_out;
    millis_state_t millis_state = {0,0}; // NEW you will see where it's used, namely..

    while (testCnt < 2000) {
        txStart = MILLIS(millis_state); // NEW ..here..
        do {
            millis_ = MILLIS(millis_state); // NEW ..and here
            not_timed_out = (millis_ - txStart) < RF69_TX_LIMIT_MS;
            debug_print ("task %d: testCnt(%d), millis(%d), txStart(%d), millis_-txStart(%d), timedOut(%d)\n",
                    task_num, testCnt, millis_, txStart, millis_ - txStart, !not_timed_out);
            delay_milliseconds(20);
        } while ((digitalRead() == 0) && not_timed_out);
        testCnt++;
    }
}

int main() {
    par {
        test_task(0);
        test_task(1);
    }
    return 0;
}

If you agree on this maybe this sums it all up?

[akp]

For sure, and no problem! It was an interesting exercise. Hope it is useful to you. Good luck!

[aclassifier]

It was! Thank you! And that this is ok with you (?)

http://www.teigfam.net/oyvind/home/tech ... urtesy_akp

Disclaimer: no money, gift or ads no nothing else than fun and expenses with any of my blog notes! And I always try to credit people!

[akp]

of course.