-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace Nordic's appTimer with a driver for RTC1 #932
Conversation
|
Please remove the project header file, see Travis log : error for #include "projectconfig.h" |
|
We should inform users who reported problems with previous implementation about this update, they can test this. |
|
@0xc0170 could you please review the code? Either you or Bogdan? I'll also connect with the previous reporters. |
| NRF_RTC1->EVTENCLR = RTC_EVTEN_OVRFLW_Msk; | ||
| } | ||
|
|
||
| static __INLINE void INVOKE_CALLBACK(void) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the function in capital letters? This can clash with macros
Replace use of Nordic's AppTimer with an RTC driver.
| * 32-bit timestamp. | ||
| */ | ||
| uint64_t timestamp64 = (RTC_UNITS_TO_MICROSECONDS(rtc1_getCounter()) & ~(uint64_t)0xFFFFFFFF) + timestamp; | ||
| if ((us_ticker_read() > 0xF0000000) && (timestamp < 0x10000000)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you describe this check ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IS there a reason to call 2x us ticker , using 2 different invokes (RTC_UNITS_TO_MICROSECONDS(rtc1_getCounter() and us_ticker_read() ) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're correct. I'm duplicating work here. I could reuse the value from RTC_UNITS_TO_MICROSECONDS(rtc1_getCounter()) instead of calling us_ticker_read(). thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ((us_ticker_read() > 0xF0000000) && (timestamp < 0x10000000)) {
the above check detects an attempt to set a timeout for a value which has wrapped around, but the attempt is being made at a time when the current RTC value when translated into microseconds hasn't wrapped around.
need to add stdbool.h
| overflowCount++; | ||
| NRF_RTC1->EVENTS_OVRFLW = 0; | ||
| NRF_RTC1->EVTENCLR = RTC_EVTEN_OVRFLW_Msk; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this duplication? The overflow is handled in rtc1_getCounter(), which is invoked meeting some conditions below. Can this be reorganized?
counter = rtc1_getCounter();
if (NRF_RTC1->EVENTS_COMPARE[0] && us_ticker_callbackPending && ((int)(us_ticker_callbackTimestamp - counter) <= 0))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it does give a (small) chance on errors with really unlucky timing. When no interrupts can be handled (for example because it is in the Ticker interrupt routine), and between the check on the overflow flag and the reading of the timer it overflows it will use the new RTC value with the old overflow value.
As random example what I did once: https://github.com/mbedmicro/mbed/blob/master/libraries/mbed/targets/hal/TARGET_Freescale/TARGET_K20XX/us_ticker.c#L74. There are probably other options too.
|
I'm using the following main.cpp to test my ticker for long periods of time. #include "mbed.h"
#include "iBeaconService.h"
BLEDevice ble;
Ticker ticker1;
Ticker ticker2;
DigitalOut led1(LED1);
DigitalOut led2(LED2);
void periodicCallback1(void)
{
led1 = !led1; /* Do blinky on LED1 while we're waiting for BLE events */
}
void periodicCallback2(void)
{
led2 = !led2; /* Do blinky on LED2 while we're waiting for BLE events */
}
const uint8_t uuid[] = {0xE2, 0x0A, 0x39, 0xF4, 0x73, 0xF5, 0x4B, 0xC4,
0xA1, 0x2F, 0x17, 0xD1, 0xAD, 0x07, 0xA9, 0x61
};
uint16_t majorNumber = 1122;
uint16_t minorNumber = 3344;
uint16_t txPower = 0xC8;
int main(void)
{
ble.init();
ticker1.attach(periodicCallback1, 0.5);
ticker2.attach(periodicCallback2, 0.25);
iBeaconService ibeacon(ble, uuid, majorNumber, minorNumber, txPower);
ble.setAdvertisingInterval(Gap::MSEC_TO_ADVERTISEMENT_DURATION_UNITS(1000)); /* 1000ms. */
ble.startAdvertising();
while(1) {
ble.waitForEvent(); // allows or low power operation
}
} |
This requires careful review and testing. I've done a fair bit, but much more is needed.
This has been very tricky code owing mostly to:
Please review and test. I have a suspicion extensive testing may reveal more hardware related quirks which haven't been taken care of.