NTP Sync may not work as intended
During the processing of a time sync request an NTP style clockOffset calculation is performed (ntp.hpp)
inline int32_t clockOffset(uint32_t time0, uint32_t time1, uint32_t time2,
uint32_t time3) {
uint32_t offset =
((int32_t)(time1 - time0) / 2) + ((int32_t)(time2 - time3) / 2);
// Take small steps to avoid over correction
if (offset < 0.5 * TASK_SECOND && offset > 4) offset = offset / 4;
return offset;
}
Which calculates the offset as uint32_t and implicitly converts to int32_t on the return statement. However the over correction disciplining logic may need review. (Assuming the intention was to moderate steps of less than 0.5 seconds and avoid integer division resulting in zero)
- TASK_SECOND is defined as 1000L (milliseconds) whereas offset is calculated in microseconds so only steps of 500 microseconds (0.5 milliseconds) will be disciplined. As the acceptable offset accuracy is +/- 5 milliseconds that does seem a very small threshold.
- The protection of offsets less than 4 seems unnecessary. Yes zero would be returned but an offset of 4 microseconds is insignificant anyway.
- Negative offsets will be represented by large uint32_t values and therefore are never disciplined.
Suggested change
#define DISCIPLINE_THRESHOLD 500000L
inline int32_t clockOffset(uint32_t time0, uint32_t time1, uint32_t time2,
uint32_t time3) {
int32_t offset =
((int32_t)(time1 - time0) / 2) + ((int32_t)(time2 - time3) / 2);
// Take small steps to avoid over correction
if (abs(offset) < DISCIPLINE_THRESHOLD) offset = offset / 4;
return offset;
Edited by gordon