Clock correction tests

[acecilia]

I extracted the clock correction logic from tdoa3 in its own engine, so it is possible to test it (and reuse it). I created several tests, and fixed some things in the updateClockCorrection() function.

The previous clock correction code is untouched for now. I would like to know if:

• This implementation and tests are interested for you and want them merged.
• This is interested for you, but some modifications need to be done in order to have it merged.
• You are not interested on this kind of modifications for now, and prefer to focus in something else.

[acecilia]

This was previously masked to 32 bits (0xFFFFFFFF) instead of 40 (0xFFFFFFFFFF). Any reason for it? I understand that, in this case, with 32 bits is enough because the timestamp represents a very short amount of time that can be represented in 32bits, but in general the truncation should be done in 40 bits, right?

[krichardsson]

Yes, both TDoA2 and TDoA3 only has 32 bits in the protocol for DWM timestamps (see https://wiki.bitcraze.io/doc:lps:tdoa3:protocol). This works as long as we transmit packets more often than the wrap time (about 67 ms) and use sequence numbers.
The current implementation is a bit dirty in that protocol properties leaks into the algorithm. I suggest that you add a parameter with a bit mask to the functions that require truncation of timestamps in your generalized implementation.

[acecilia]

It took me some time to see why using 40 bits would throw a wrong result, but I finally got it.
An unrelated question: why in calcClockCorrection in the tdoa3 engine you use uint64_t instead of uint32_t?

[acecilia]

I moved the spec check to the start of the function, so there is no processing of the sample if it is not inside the specs range

[acecilia]

In previous implementation, when the sample was accepted after the bucket was emptied, fillClockCorrectionBucket(storage) and sampleIsAccepted = truewhere not called. As a result, the clock was set, but the function was returning false.

[krichardsson]

I think it can be useful to extract the clock correction functionality. A few comments before I merge it though.

1. The pattern you use is very clean but slightly verbose in terms of files. It is not completely inline with the current code base but I think it is an interesting experiment to get a feel for the implications of using this style of coding. It adds a lot of benefits for testability and clear APIs which is something that I like.
2. Since this is fairly generic functionality I suggest that you move it to utils and put the files in sub folders to group them, for instance src/utils/interface/clockcorrection and src/utils/src/clockcorrection
3. There is a slight performance penalty in that we do not have link time inlining turned on (I think). This is not a problem in this case but if we continue to use this patten we should probably consider turning on link time optimizations.
4. See comment on truncation of timestamps.

Good job!

[acecilia]

There you go :)
About your point 1. Yes I agree. I have the feeling that latelly, the software community is moving towards smaller files with few and clear responsabilities (which are easier to test), even if that means increasing the number of them. I agree with that idea. The problem I see in this case is that in makefiles is a bit more difficult to manage a larger number of files, but I think for now is fine.

[krichardsson]

I have done some testing and I found 3 issues

1. The argument in the getClockCorrection() function should be const.
2. In the updateClockCorrection() function, when the check if the clock correction candidate is within specs was moved to the outer level, the bucket is no longer emptied when the candidate is out of spec.
3. In the updateClockCorrection() function, in the case when the difference is outside the accepted noise level, a call to fillClockCorrectionBucket(storage) has been added as well as accepting the sample. The thinking in the original implementation was that we should not trust the sample unless we have got two or more samples that are similar (within the accepted noise level) and has been LP filtered. When testing the two solutions the old implementation performs better and I suggest that you revert the change.

[acecilia]

I just added logging capabilities for the clock correction engine. Thanks to it I realised that between two of my drones, the clock correction is going out of the specs values. Did you experience something like this?

See attached video: the clock correction candidate grows (due to temperature I guess) and surpasses the maximum spec limit.

ClockCorrection.mov.zip

[krichardsson]

Sorry for the delay... Thanks!