all code review fixes

src/time/cuc.rs

@@ -39,7 +39,7 @@ pub const MAX_CUC_LEN_SMALL_PREAMBLE: usize = 8;
 pub enum FractionalResolution {
     /// No fractional part, only second resolution
     Seconds = 0,
-    /// 256 fractional parts, resulting in 1/255 ~= 4 ms fractional resolution
+    /// 255 fractional parts, resulting in 1/255 ~= 4 ms fractional resolution
     FourMs = 1,
     /// 65535 fractional parts, resulting in 1/65535 ~= 15 us fractional resolution
     FifteenUs = 2,
@@ -67,12 +67,16 @@ impl TryFrom<u8> for FractionalResolution {
 #[inline]
 pub fn convert_fractional_part_to_ns(fractional_part: FractionalPart) -> u64 {
     let div = fractional_res_to_div(fractional_part.0);
-    assert!(fractional_part.1 < div);
+    assert!(fractional_part.1 <= div);
     10_u64.pow(9) * fractional_part.1 as u64 / div as u64
 }
 
 #[inline(always)]
 pub const fn fractional_res_to_div(res: FractionalResolution) -> u32 {
+    // We do not use the full possible range for a given resolution. This is because if we did
+    // that, the largest value would be equal to the counter being incremented by one. Thus, the
+    // smallest allowed fractions value is 0 while the largest allowed fractions value is the
+    // closest fractions value to the next counter increment.
     2_u32.pow(8 * res as u32) - 1
 }
 
@@ -92,19 +96,9 @@ pub fn fractional_part_from_subsec_ns(
         panic!("passed nanosecond value larger than 1 second");
     }
     let resolution = fractional_res_to_div(res) as u64;
-    // Use integer division because this can reduce code size of really small systems.
-    // First determine the nanoseconds for the smallest segment given the resolution.
-    // Then divide by that to find out the fractional part. For the calculation of the smallest
-    // fraction, we perform a ceiling division. This is because if we would use the default
-    // flooring division, we would divide by a smaller value, thereby allowing the calculation to
-    // invalid fractional parts which are too large. For the division of the nanoseconds by the
-    // smallest fraction, a flooring division is correct.
-    // The multiplication with 100000 is necessary to avoid precision loss during integer division.
-    // TODO: Floating point division might actually be faster option, but requires additional
-    //       code on small embedded systems..
-    let fractional_part = ns * 100000 / ((sec_as_ns * 100000 + resolution) / resolution);
-    // Floating point division.
-    //let fractional_part = (ns as f64 / ((sec_as_ns as f64) / resolution as f64)).floor() as u32;
+    // This is probably the cheapest way to calculate the fractional part without using expensive
+    // floating point division.
+    let fractional_part = ns * (resolution + 1) / sec_as_ns;
     Some(FractionalPart(res, fractional_part as u32))
 }
 
@@ -294,16 +288,22 @@ impl CucTime {
         leap_seconds: u32,
     ) -> Result<Self, StdTimestampError> {
         let now = SystemTime::now().duration_since(SystemTime::UNIX_EPOCH)?;
-        let ccsds_epoch = unix_epoch_to_ccsds_epoch(now.as_secs() as i64);
-        ccsds_epoch
-            .checked_add(i64::from(leap_seconds))
+        let mut counter =
+            u32::try_from(unix_epoch_to_ccsds_epoch(now.as_secs() as i64)).map_err(|_| {
+                TimestampError::Cuc(CucError::InvalidCounter {
+                    width: 4,
+                    counter: now.as_secs(),
+                })
+            })?;
+        counter = counter
+            .checked_add(leap_seconds)
             .ok_or(TimestampError::Cuc(CucError::LeapSecondCorrectionError))?;
         if fraction_resolution == FractionalResolution::Seconds {
-            return Ok(Self::new(ccsds_epoch as u32));
+            return Ok(Self::new(counter));
         }
         let fractions =
             fractional_part_from_subsec_ns(fraction_resolution, now.subsec_nanos() as u64);
-        Self::new_with_fractions(ccsds_epoch as u32, fractions.unwrap())
+        Self::new_with_fractions(counter, fractions.unwrap())
             .map_err(|e| StdTimestampError::Timestamp(e.into()))
     }
 
@@ -318,7 +318,8 @@ impl CucTime {
     pub fn update_from_now(&mut self, leap_seconds: u32) -> Result<(), StdTimestampError> {
         let now = SystemTime::now().duration_since(SystemTime::UNIX_EPOCH)?;
         self.counter.1 = unix_epoch_to_ccsds_epoch(now.as_secs() as i64) as u32;
-        self.counter
+        self.counter.1 = self
+            .counter
             .1
             .checked_add(leap_seconds)
             .ok_or(TimestampError::Cuc(CucError::LeapSecondCorrectionError))?;
@@ -360,17 +361,17 @@ impl CucTime {
         res: FractionalResolution,
         leap_seconds: u32,
     ) -> Result<Self, TimestampError> {
-        let ccsds_epoch = unix_epoch_to_ccsds_epoch(unix_stamp.secs);
+        let mut counter = unix_epoch_to_ccsds_epoch(unix_stamp.secs);
         // Negative CCSDS epoch is invalid.
-        if ccsds_epoch < 0 {
+        if counter < 0 {
             return Err(TimestampError::DateBeforeCcsdsEpoch(*unix_stamp));
         }
-        ccsds_epoch
+        counter = counter
             .checked_add(i64::from(leap_seconds))
             .ok_or(TimestampError::Cuc(CucError::LeapSecondCorrectionError))?;
         let fractions =
             fractional_part_from_subsec_ns(res, unix_stamp.subsec_millis() as u64 * 10_u64.pow(6));
-        Self::new_generic(WidthCounterPair(4, ccsds_epoch as u32), fractions).map_err(|e| e.into())
+        Self::new_generic(WidthCounterPair(4, counter as u32), fractions).map_err(|e| e.into())
     }
 
     pub fn new_u16_counter(counter: u16) -> Self {
@@ -1194,13 +1195,53 @@ mod tests {
     }
 
     #[test]
-    fn assert_largest_fractions() {
+    fn test_small_fraction_floored_to_zero() {
+        let fractions =
+            fractional_part_from_subsec_ns(FractionalResolution::SixtyNs, 59)
+                .unwrap();
+        assert_eq!(fractions.1, 0);
+    }
+
+    #[test]
+    fn test_small_fraction_becomes_fractional_part() {
+        let fractions =
+            fractional_part_from_subsec_ns(FractionalResolution::SixtyNs, 61)
+                .unwrap();
+        assert_eq!(fractions.1, 1);
+    }
+
+    #[test]
+    fn test_smallest_resolution_small_nanoseconds_floored_to_zero() {
+        let fractions =
+            fractional_part_from_subsec_ns(FractionalResolution::FourMs, 3800 * 1e3 as u64)
+                .unwrap();
+        assert_eq!(fractions.1, 0);
+    }
+
+    #[test]
+    fn test_smallest_resolution_small_nanoseconds_becomes_one_fraction() {
+        let fractions =
+            fractional_part_from_subsec_ns(FractionalResolution::FourMs, 4000 * 1e3 as u64)
+                .unwrap();
+        assert_eq!(fractions.1, 1);
+    }
+
+    #[test]
+    fn test_smallest_resolution_large_nanoseconds_becomes_largest_fraction() {
+        let fractions =
+            fractional_part_from_subsec_ns(FractionalResolution::FourMs, 10u64.pow(9) - 1)
+                .unwrap();
+        assert_eq!(fractions.1, 2_u32.pow(8) - 1);
+    }
+
+    #[test]
+    fn test_largest_fractions_with_largest_resolution() {
         let fractions =
             fractional_part_from_subsec_ns(FractionalResolution::SixtyNs, 10u64.pow(9) - 1)
                 .unwrap();
         // The value can not be larger than representable by 3 bytes
         // Assert that the maximum resolution can be reached
-        assert_eq!(fractions.1, 2_u32.pow(3 * 8) - 2);
+        assert_eq!(fractions.1, 2_u32.pow(3 * 8) - 1);
     }
 
     fn check_stamp_after_addition(cuc_stamp: &CucTime) {
@@ -1212,7 +1253,7 @@ mod tests {
         assert_eq!(cuc_stamp.width_counter_pair().1, 202);
         let fractions = cuc_stamp.width_fractions_pair().unwrap().1;
         let expected_val =
-            (0.5 * fractional_res_to_div(FractionalResolution::FifteenUs) as f64).floor() as u32;
+            (0.5 * fractional_res_to_div(FractionalResolution::FifteenUs) as f64).ceil() as u32;
         assert_eq!(fractions, expected_val);
         let cuc_stamp2 = cuc_stamp + Duration::from_millis(501);
         // What I would roughly expect
@@ -1300,7 +1341,7 @@ mod tests {
                 .expect("failed to create cuc from unix stamp");
         assert_eq!(
             cuc.counter(),
-            (-DAYS_CCSDS_TO_UNIX * SECONDS_PER_DAY as i32) as u32
+            (-DAYS_CCSDS_TO_UNIX * SECONDS_PER_DAY as i32) as u32 + LEAP_SECONDS
         );
     }
 

src/time/mod.rs

@@ -371,16 +371,16 @@ impl UnixTime {
     }
 
     // Calculate the difference in milliseconds between two UnixTimestamps
-    pub fn diff_in_millis(&self, other: &UnixTime) -> i64 {
-        let seconds_difference = self.secs - other.secs;
+    pub fn diff_in_millis(&self, other: &UnixTime) -> Option<i64> {
+        let seconds_difference = self.secs.checked_sub(other.secs)?;
         // Convert seconds difference to milliseconds
-        let milliseconds_difference = seconds_difference * 1000;
+        let milliseconds_difference = seconds_difference.checked_mul(1000)?;
 
         // Calculate the difference in subsecond milliseconds directly
         let subsecond_difference_nanos = self.subsec_nanos as i64 - other.subsec_nanos as i64;
 
         // Combine the differences
-        milliseconds_difference + (subsecond_difference_nanos / 1_000_000)
+        Some(milliseconds_difference + (subsecond_difference_nanos / 1_000_000))
     }
 }
 
@@ -447,11 +447,11 @@ pub struct StampDiff {
 }
 
 impl Sub for UnixTime {
-    type Output = StampDiff;
+    type Output = Option<StampDiff>;
 
     fn sub(self, rhs: Self) -> Self::Output {
-        let difference = self.diff_in_millis(&rhs);
-        if difference < 0 {
+        let difference = self.diff_in_millis(&rhs)?;
+        Some(if difference < 0 {
             StampDiff {
                 positive_duration: false,
                 duration_absolute: Duration::from_millis(-difference as u64),
@@ -461,7 +461,7 @@ impl Sub for UnixTime {
                 positive_duration: true,
                 duration_absolute: Duration::from_millis(difference as u64),
             }
-        }
+        })
     }
 }
 
@@ -690,7 +690,7 @@ mod tests {
         let StampDiff {
             positive_duration,
             duration_absolute,
-        } = stamp_later - UnixTime::new(1, 0);
+        } = (stamp_later - UnixTime::new(1, 0)).expect("stamp diff error");
         assert!(positive_duration);
         assert_eq!(duration_absolute, Duration::from_secs(1));
     }
@@ -702,7 +702,7 @@ mod tests {
         let StampDiff {
             positive_duration,
             duration_absolute,
-        } = stamp_later - stamp_earlier;
+        } = (stamp_later - stamp_earlier).expect("stamp diff error");
         assert!(positive_duration);
         assert_eq!(duration_absolute, Duration::from_millis(1900));
     }
@@ -714,7 +714,7 @@ mod tests {
         let StampDiff {
             positive_duration,
             duration_absolute,
-        } = stamp_earlier - stamp_later;
+        } = (stamp_earlier - stamp_later).expect("stamp diff error");
         assert!(!positive_duration);
         assert_eq!(duration_absolute, Duration::from_millis(1900));
     }