Add API function to trigger synchronous PPI event

[arzippi]

I added new functionality to the time_sync API in order to inject a PPI event that will be triggered based on the synchronized timebase. See issue #7

[arzippi]

I had a little of trouble when I merged back my code into your example application. I was able to get it running and tested it with one master and one receiver. You have to use the main.c from ble_central for the master and the main.c from ble_peripheral for the receiver.

The trick is to start the synchronization on the master before the receiver reaches it's first trigger point (at 5500 ticks). After that, they run perfectly in sync. I did this by starting the receiver first, then quickly switching on the master and starting the synchronization. This is necessary for now, because usually the master should send a trigger request with a valid timestamp in the future to the receiver via BLE. It didn't have time to implement this into the example application yet...

[nordic-auko]

Thanks, @arzippi!

I finally had a chance to test the code now. So far I've only tested without Bluetooth, assessing the accuracy of the "old" functionality of always triggering a GPIO when the timer wraps around, and the accuracy of the new ts_set_trigger() function. No issues found so far, and the accuracy and stability looks great!

I've let it run overnight with an oscilloscope attached and haven't seen any outliers or clock drift. In my test setup I use ts_set_trigger() to toggle a GPIOTE at 500 Hz and observe the delay of the toggling on two nRF52-DKs sitting on my desk. The average delay is ~66 ns, and the delay is within the [-66, +185] range so far. The original code had a much wider range, I believe.

I will do some more testing to check the interoperability with Bluetooth.

I have some observations/questions: 1) ts_set_trigger() does not work properly when target_tick is set to evt->targetTick + 1 . I haven't looked into the cause yet, but I suspect it is not trivial to fix. I'm thinking of returning an error if target_tick is <= current_time + 1. 2) Have you experimented with lower values for TIME_SYNC_TIMER_MAX_VAL? I'm thinking it might make sense to set the default value to 8000 such that the timebase is 1 tick = 1 ms. Regardless I'm planning to add a macro that handles the conversion.

After further testing, my plan is to merge the PR and follow up with a commit to clean up documentation, some tab/space stuff, and some naming to conform with the rest of the code. I like the changes you made to the API, so the only changes in the header file I'm thinking of doing is adding documentation and changing some variable names (e.g. startTick, targetTick -> tick_start, tick_target). This shouldn't create any significant conflicts for you if you decide to rebase your fork?

[nordic-auko]

I had a little of trouble when I merged back my code into your example application. I was able to get it running and tested it with one master and one receiver. You have to use the main.c from ble_central for the master and the main.c from ble_peripheral for the receiver.

The trick is to start the synchronization on the master before the receiver reaches it's first trigger point (at 5500 ticks). After that, they run perfectly in sync. I did this by starting the receiver first, then quickly switching on the master and starting the synchronization. This is necessary for now, because usually the master should send a trigger request with a valid timestamp in the future to the receiver via BLE. It didn't have time to implement this into the example application yet...

Ideally the receiver would be notified when there is a jump in the timebase due to new packets being received, that shifts the local time past the trigger time. I might leave this as a "TODO" for now, and look at some simple workarounds in the application. For example, set an app_timer as a "watchdog" that simply calls ts_set_trigger() again if a trigger is missed.

[arzippi]

Hi @nordic-auko,

sounds very good!

Here are my comments to your questions:

ts_set_trigger() does not work properly when target_tick is set to evt->targetTick + 1 . I haven't looked into the cause yet, but I suspect it is not trivial to fix. I'm thinking of returning an error if target_tick is <= current_time + 1.

-> I think this should work, as long as the timer capture is updated before the timer reaches the value. But I can try and look into this...

Have you experimented with lower values for TIME_SYNC_TIMER_MAX_VAL? I'm thinking it might make sense to set the default value to 8000 such that the timebase is 1 tick = 1 ms. Regardless I'm planning to add a macro that handles the conversion.

-> No, I haven't changed the TIME_SYNC_TIMER_MAX_VAL. I think this is something I already mentioned in one of my comments in the original issue. The CHECK_INVALID_TIMING macro adds a relativly large "safety zone" and if you decrease the TIME_SYNC_TIMER_MAX_VAL, the usable timeslots become smaller and smaller.

I like the changes you made to the API, so the only changes in the header file I'm thinking of doing is adding documentation and changing some variable names (e.g. startTick, targetTick -> tick_start, tick_target). This shouldn't create any significant conflicts for you if you decide to rebase your fork?

-> I tried to conform to your style, seems like I forgot to do this in the header...

[nordic-auko]

Have you experimented with lower values for TIME_SYNC_TIMER_MAX_VAL? I'm thinking it might make sense to set the default value to 8000 such that the timebase is 1 tick = 1 ms. Regardless I'm planning to add a macro that handles the conversion.

-> No, I haven't changed the TIME_SYNC_TIMER_MAX_VAL. I think this is something I already mentioned in one of my comments in the original issue. The CHECK_INVALID_TIMING macro adds a relativly large "safety zone" and if you decrease the TIME_SYNC_TIMER_MAX_VAL, the usable timeslots become smaller and smaller.

Ah, yes, I recall this discussion now. I'll leave it at 2.5 ms and add an #error if it is set to value 10000 or less.

Regarding API changes, I think it would be useful to extend the callback event type. By having the following events the receivers will know when the ts_trigger API can be used safely:

typedef enum
{
    TS_EVT_SYNCHRONIZED,
    TS_EVT_DESYNCHRONIZED,
    TS_EVT_TRIGGERED,
} ts_evt_type_t;

typedef struct
{
    ts_evt_type_t type;
    union
    {
        struct
        {
            uint32_t startTick;
            uint32_t targetTick;
            uint32_t lastSync;
            uint16_t syncPacketCnt;
            uint16_t usedPacketCnt;
        } triggered;
    } params;
} ts_evt_t;

TS_EVT_SYNCHRONIZED happens when the first sync packet is received and successfully processed. TS_EVT_DESYNCHRONIZED happens when there is more than #define microseconds since the previously received packet.

[arzippi]

Regarding API changes, I think it would be useful to extend the callback event type. By having the following events the receivers will know when the ts_trigger API can be used safely:

Yes, this sounds like a good idea. However, I think using pointers in the union is probably better. Otherwise the data will always be copied.

[arzippi]

Do you want me to change some more stuff in my forked branch (like the variable names in the header), or do you plan to merge it directly and cleanup afterwards?

[nordic-auko]

Do you want me to change some more stuff in my forked branch (like the variable names in the header), or do you plan to merge it directly and cleanup afterwards?

I was thinking of merging directly, and follow up with a commit that does minor cleanup and adds the mentioned API changes

[nordic-auko]

Thanks again, @arzippi ! I think this is a really useful feature you added here.

As mentioned, I've pushed a commit directly to master now with some minor cleanup and new features based on your work