Algorithm support for periodic sources. #1768
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1768 +/- ##
==========================================
+ Coverage 81.60% 82.01% +0.40%
==========================================
Files 66 66
Lines 20671 21220 +549
==========================================
+ Hits 16869 17403 +534
- Misses 3802 3817 +15 ☔ View full report in Codecov by Sentry. |
davidv1992
force-pushed
the
algorithm-periodic-source
branch
from
2df1ed3 to
3ccb6ae
Compare
davidv1992
force-pushed
the
algorithm-periodic-source
branch
from
3ccb6ae to
cda78f3
Compare
| @@ -34,6 +34,7 @@ struct SourceSnapshot<Index: Copy> { | |||
| state: KalmanState, | |||
| wander: f64, | |||
| delay: f64, | |||
| period: Option<f64>, | |||
There was a problem hiding this comment.
I think we should document somewhere what period actually means.
| @@ -24,6 +24,11 @@ pub(super) fn select<Index: Copy>( | |||
| let mut bounds: Vec<(f64, BoundType)> = Vec::with_capacity(2 * candidates.len()); | |||
|
|
|||
| for snapshot in candidates.iter() { | |||
| if snapshot.period.is_some() { | |||
There was a problem hiding this comment.
Although nothing would particularly go wrong (i.e. I think we would just not synchronize at all), we could now have the situation where we only have periodic sources. Either we should probably require at least one non-periodic source, or we could have some (not that great) option that uses our current clock time for periodic sources (but that would require a whole bunch of additional changes probably).
| if cur > max { | ||
| max = cur; | ||
| maxt = *time; | ||
| BoundType::Start => { |
There was a problem hiding this comment.
I think we should probably update the comment on top of this function (and/or maybe add a few inline comments), to explain why we now look for both the lower and higher bound.
| } | ||
| } | ||
|
|
||
| // Catch programming errors. If this ever fails there is high risk of missteering, better fail hard in that case | ||
| assert_eq!(maxlow, maxhigh); |
There was a problem hiding this comment.
Maybe add a third parameter for the panic message? Or at least explain 'this' in the comment?
| // Catch programming errors. If this ever fails there is high risk of missteering, better fail hard in that case | ||
| assert_eq!(maxlow, maxhigh); | ||
| let max = maxlow; | ||
|
|
||
| if max >= synchronization_config.minimum_agreeing_sources && max * 4 > bounds.len() { |
There was a problem hiding this comment.
I think we should probably update the documentation of minimum agreeing sources to indicate that it refers to non-periodic sources only?
No description provided.