From f3610650482fcc4fdf10d7d57f478c81d7ae7ae0 Mon Sep 17 00:00:00 2001
From: Austin Gill <Notgnoshi@gmail.com>
Date: Tue, 8 Aug 2023 16:02:30 -0500
Subject: [PATCH] Require the CAN frame type when creating a CanId

The most reliable way to know if a CAN ID is standard or extended is for
the driver to determine this (based on the IDE bit, which isn't
contained in the 29-bit ID). So we require the driver to determine the
frame type when creating the ID.

This was supposed to have been merged as a part of [1], but because of a
rebase-and-force-push snafu, probably as a result of poor Git hygiene on
my part, these changes were overwritten before the PR was merged.

[1] https://github.com/Open-Agriculture/AgIsoStack-rs/pull/4
---
 src/driver/can_id.rs | 59 +++++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/src/driver/can_id.rs b/src/driver/can_id.rs
index 57010da..0a791ed 100644
--- a/src/driver/can_id.rs
+++ b/src/driver/can_id.rs
@@ -53,24 +53,40 @@ pub enum Type {
 #[repr(transparent)]
 pub struct CanId(u32);
 
+// Linux uses the top three unused bits to indicate whether the frame is standard/extended, remote,
+// or an error frame. We do the same, because it's convenient.
+const CAN_EFF_FLAG: u32 = 0x80000000;
+// const CAN_RTR_FLAG: u32 = 0x40000000;
+// const CAN_ERR_FLAG: u32 = 0x20000000;
+
+const CAN_EFF_MASK: u32 = 0x1FFFFFFF;
+const CAN_SFF_MASK: u32 = 0x000007FF;
+
 impl CanId {
-    pub fn new(raw: u32) -> Self {
+    pub fn new(raw: u32, type_: Type) -> Self {
+        let raw = match type_ {
+            Type::Extended => (raw & CAN_EFF_MASK) | CAN_EFF_FLAG,
+            Type::Standard => raw & CAN_SFF_MASK,
+        };
         Self(raw)
     }
 
     /// Get the raw value of the CAN ID
     #[inline]
     pub fn raw(&self) -> u32 {
-        self.0
+        match self.type_() {
+            Type::Extended => self.0 & CAN_EFF_MASK,
+            Type::Standard => self.0 & CAN_SFF_MASK,
+        }
     }
 
     /// Get the type of the ID (standard or extended)
     #[inline]
     pub fn type_(&self) -> Type {
-        if self.raw() <= 0x7FF {
-            Type::Standard
-        } else {
+        if self.0 & CAN_EFF_FLAG != 0 {
             Type::Extended
+        } else {
+            Type::Standard
         }
     }
 
@@ -123,62 +139,53 @@ impl CanId {
 mod tests {
     use super::*;
 
-    #[test]
-    fn test_standard_extended() {
-        let can_id = CanId::new(0x07FF);
-        assert_eq!(can_id.type_(), Type::Standard);
-
-        let can_id = CanId::new(0x08FF);
-        assert_eq!(can_id.type_(), Type::Extended);
-    }
-
     #[test]
     fn test_priority() {
-        let can_id = CanId::new(0x18EF1CF5);
+        let can_id = CanId::new(0x18EF1CF5, Type::Extended);
         assert_eq!(can_id.priority(), Priority::Default);
     }
 
     #[test]
     fn test_source_address() {
-        let can_id = CanId::new(0x0705);
+        let can_id = CanId::new(0x0705, Type::Standard);
         assert_eq!(can_id.type_(), Type::Standard);
         // TODO: Is this right? Do 11-bit IDs always have a global address?
         assert_eq!(can_id.source_address(), Address::GLOBAL);
 
-        let can_id = CanId::new(0x18EF1CF5);
+        let can_id = CanId::new(0x18EF1CF5, Type::Extended);
         assert_eq!(can_id.source_address(), Address(0xF5));
     }
 
     #[test]
     fn test_destination_address() {
-        let can_id = CanId::new(0x0705);
+        let can_id = CanId::new(0x0705, Type::Standard);
         assert_eq!(can_id.destination_address(), Address::GLOBAL);
 
-        let can_id = CanId::new(0x18EEFF1C);
+        let can_id = CanId::new(0x18EEFF1C, Type::Extended);
         assert_eq!(can_id.destination_address(), Address::GLOBAL);
 
-        let can_id = CanId::new(0x09F8031C);
+        let can_id = CanId::new(0x09F8031C, Type::Extended);
         assert_eq!(can_id.destination_address(), Address::GLOBAL);
 
-        let can_id = CanId::new(0x0CAC1C13);
+        let can_id = CanId::new(0x0CAC1C13, Type::Extended);
         assert_eq!(can_id.destination_address(), Address(0x1C));
     }
 
     #[test]
     fn test_pgn() {
-        let can_id = CanId::new(0x07FF);
+        let can_id = CanId::new(0x07FF, Type::Standard);
         assert_eq!(can_id.pgn(), Pgn::NULL);
 
-        let can_id = CanId::new(0x0CAC1C13);
+        let can_id = CanId::new(0x0CAC1C13, Type::Extended);
         assert_eq!(can_id.pgn(), Pgn::from_raw(0x0AC00));
 
-        let can_id = CanId::new(0x18FF3F13);
+        let can_id = CanId::new(0x18FF3F13, Type::Extended);
         assert_eq!(can_id.pgn(), Pgn::from_raw(0x0FF3F));
 
-        let can_id = CanId::new(0x18EF1CF5);
+        let can_id = CanId::new(0x18EF1CF5, Type::Extended);
         assert_eq!(can_id.pgn(), Pgn::from_raw(0x0EF00));
 
-        let can_id = CanId::new(0x18EEFF1C);
+        let can_id = CanId::new(0x18EEFF1C, Type::Extended);
         assert_eq!(can_id.pgn(), Pgn::from_raw(0x0EE00));
     }
 }