Fixing warnings, improving code quality

erdemsimsek · erdemsimsek · commit d8f59d076c9d · 2025-10-12T14:49:52.000+01:00
diff --git a/canopen-common/src/sdo.rs b/canopen-common/src/sdo.rs
@@ -1,7 +1,6 @@
 // sdo.rs - Updated for the new connection architecture
-use socketcan::{CanSocket, Socket, CanFrame, StandardId};
+use socketcan::{CanFrame, StandardId};
 use socketcan::EmbeddedFrame as Frame;
-use std::time::{Duration, Instant};
 use std::error::Error;
 use std::fmt;
 
@@ -24,17 +23,7 @@ pub enum SdoCommand {
 }
 
 impl SdoCommand {
-    fn from_u8(value: u8) -> Option<Self> {
-        match value & 0xE0 {  // Mask the command specifier bits
-            0x40 => Some(Self::InitiateUploadRequest),
-            0x60 => Some(Self::UploadSegmentRequest),
-            0x00 => Some(Self::UploadSegmentResponse),
-            0x80 => Some(Self::AbortTransfer),
-            _ => None,
-        }
-    }
-
-    fn is_expedited_response(value: u8) -> bool {
+    pub(crate) fn is_expedited_response(value: u8) -> bool {
         (value & 0xE0) == 0x40 && (value & 0x02) != 0
     }
 }
@@ -68,15 +57,6 @@ impl SdoDataType {
             _ => None,
         }
     }
-
-    fn size_bytes(&self) -> Option<usize> {
-        match self {
-            Self::UInt8 | Self::Int8 => Some(1),
-            Self::UInt16 | Self::Int16 => Some(2),
-            Self::UInt32 | Self::Int32 | Self::Real32 => Some(4),
-            Self::VisibleString | Self::OctetString => None, // Variable length
-        }
-    }
 }
 
 /// SDO Request structure
diff --git a/canopen-viewer/src/canopen/connect.rs b/canopen-viewer/src/canopen/connect.rs
@@ -3,16 +3,17 @@ use socketcan::{CanSocket, Socket, CanFrame, StandardId, EmbeddedFrame};
 use std::collections::HashMap;
 use std::sync::{Arc, Mutex};
 use std::time::Duration;
-use tokio::sync::{mpsc, oneshot, RwLock};
+use tokio::sync::{mpsc, oneshot};
 use tokio::task::JoinHandle;
 use std::error::Error;
 use std::fmt;
 
-use canopen_common::{SdoRequest, SdoResponse, SdoError, create_sdo_request_frame, parse_sdo_response};
+use canopen_common::{SdoRequest, SdoResponse, SdoError, parse_sdo_response};
 
 #[derive(Debug)]
 pub enum CANopenError {
     SocketError(String),
+    #[allow(dead_code)]  // Reserved for future use
     NodeNotConnected(u8),
     RequestFailed(String),
 }
@@ -47,6 +48,7 @@ enum ConnectionMessage {
         node_id: u8,
         response_tx: oneshot::Sender<Result<(), CANopenError>>,
     },
+    #[allow(dead_code)]  // Reserved for future cleanup functionality
     RemoveNode {
         node_id: u8,
         response_tx: oneshot::Sender<Result<(), CANopenError>>,
@@ -63,7 +65,6 @@ struct PendingSdoRequest {
 
 /// Per-node state management
 struct NodeState {
-    node_id: u8,
     // Queue of pending SDO requests (FIFO)
     pending_requests: std::collections::VecDeque<PendingSdoRequest>,
     // Currently active request (if any)
@@ -73,9 +74,8 @@ struct NodeState {
 }
 
 impl NodeState {
-    fn new(node_id: u8, timeout: Duration) -> Self {
+    fn new(_node_id: u8, timeout: Duration) -> Self {
         Self {
-            node_id,
             pending_requests: std::collections::VecDeque::new(),
             active_request: None,
             timeout,
diff --git a/canopen-viewer/src/canopen/mod.rs b/canopen-viewer/src/canopen/mod.rs
@@ -4,10 +4,8 @@ pub mod connect;
 // SDO protocol is now in the common library
 // Re-export from canopen-common for backwards compatibility
 pub use canopen_common::{
-    SdoRequest, SdoResponse, SdoResponseData, SdoDataType, SdoError,
-    create_sdo_request_frame, parse_sdo_response, parse_payload,
-    get_abort_code_description, SdoCommand
+    SdoRequest, SdoDataType
 };
 
-pub use connect::{CANopenConnection, CANopenNodeHandle, CANopenError};
+pub use connect::{CANopenConnection, CANopenNodeHandle};
 
diff --git a/canopen-viewer/src/communication.rs b/canopen-viewer/src/communication.rs
@@ -2,24 +2,25 @@ use std::sync::mpsc::{Receiver, Sender};
 use std::path::PathBuf;
 use configparser::ini::Ini;
 use std::collections::{BTreeMap, HashMap};
-use rand::Rng;
-use tokio::{sync::{mpsc as tokio_mpsc, oneshot}, task::JoinHandle};
+use tokio::task::JoinHandle;
 use std::time::Duration;
 use crate::canopen::{
     CANopenConnection, CANopenNodeHandle,
-    SdoRequest, SdoDataType, SdoError
+    SdoRequest, SdoDataType
 };
 
 
 #[derive(Debug, Clone)]
 pub struct SdoSubObject {
+    #[allow(dead_code)]  // Stored from EDS for reference
     pub sub_index: u8,
     pub name: String,
     pub data_type: String,
 }
 
 #[derive(Debug, Clone)]
 pub struct SdoObject {
+    #[allow(dead_code)]  // Used internally by BTreeMap, needed for EDS parsing
     pub index: u16,
     pub name: String,
     /// We use a BTreeMap to keep sub-objects automatically sorted by their sub_index (the u8 key).
@@ -29,6 +30,7 @@ pub struct SdoObject {
 #[derive(Debug, Clone, Eq, Hash, PartialEq)]
 pub struct SdoAddress {
     pub index: u16,
+    #[allow(dead_code)]  // Used in HashMap key, accessed via pattern matching
     pub sub_index: u8,
 }
 
@@ -39,37 +41,24 @@ pub enum Command {
     Subscribe {
         address: SdoAddress,
         interval_ms: u64,
+        data_type: SdoDataType,
     },
     Unsubscribe(SdoAddress),
 }
 
 #[derive(Debug)]
 pub enum Update {
     SdoList(BTreeMap<u16, SdoObject>),
+    #[allow(dead_code)]  // TODO: Will be used in Priority 1 fixes for connection status
     ConnectionSuccess(BTreeMap<u16, SdoObject>),
+    #[allow(dead_code)]  // TODO: Will be used in Priority 1 fixes for error reporting
     ConnectionFailed(String),
     SdoData {
         address: SdoAddress,
         value: String,
     },
 }
 
-async fn simulation_task(address: SdoAddress, interval_ms: u64, update_tx: Sender<Update>) {
-    println!("Starting simulation task for address {:?} with interval {} ms", &address, interval_ms);
-    let mut interval = tokio::time::interval(std::time::Duration::from_millis(interval_ms));
-
-
-    loop {
-        interval.tick().await;
-        let mut rng = rand::thread_rng();
-        let random_value = rng.gen_range(0..100);
-        let _ = update_tx.send(Update::SdoData {
-            address: address.clone(),
-            value: format!("{}", random_value),
-        });
-    }
-}
-
 async fn sdo_polling_task(
     address: SdoAddress,
     interval_ms: u64,
@@ -113,7 +102,8 @@ pub fn communication_thread_main(
 
     let rt = tokio::runtime::Runtime::new().unwrap();
     let mut subscription_handles: HashMap<SdoAddress, JoinHandle<()>> = HashMap::new();
-    let mut connection: Option<CANopenConnection> = None;
+    // Keep connection alive - it owns the background CAN reader task
+    let mut _connection_handle: Option<CANopenConnection> = None;
     let mut node_handle: Option<CANopenNodeHandle> = None;
 
 
@@ -126,7 +116,7 @@ pub fn communication_thread_main(
                     Ok::<(CANopenConnection, CANopenNodeHandle), Box<dyn std::error::Error>>((conn, handle))
                 }){
                     Ok((conn, handle)) => {
-                        connection = Some(conn);
+                        _connection_handle = Some(conn);
                         node_handle = Some(handle);
                     },
                     Err(err) => {
@@ -149,7 +139,7 @@ pub fn communication_thread_main(
                     let _ = update_tx.send(Update::SdoList(BTreeMap::new()));
                 }
             },
-            Command::Subscribe { address, interval_ms } => {
+            Command::Subscribe { address, interval_ms, data_type } => {
                 if let Some(ref handle) = node_handle {
                     println!("Subscribing to address {:?} with interval {} ms", &address, interval_ms);
 
@@ -161,7 +151,7 @@ pub fn communication_thread_main(
                         interval_ms,
                         update_tx_clone,
                         handle_clone,
-                        SdoDataType::Real32,
+                        data_type,
                     ));
 
                     subscription_handles.insert(address, subscription_handle);
@@ -177,7 +167,6 @@ pub fn communication_thread_main(
                     subscription_handle.abort();
                 }
             }
-            _ => {}
         }
     }
 }
diff --git a/canopen-viewer/src/main.rs b/canopen-viewer/src/main.rs
@@ -4,8 +4,8 @@ mod communication;
 mod canopen;
 
 use std::collections::{BTreeMap, HashMap, VecDeque};
-use std::ops::Deref;
 use communication::{Command, Update, SdoAddress, SdoObject};
+use canopen_common::SdoDataType;
 
 use eframe::{egui, NativeOptions, egui::Color32, egui::ColorImage};
 use std::process::Command as process_command;
@@ -34,15 +34,6 @@ struct ScreenshotInfo {
     rect: egui::Rect,
 }
 
-impl ScreenshotInfo {
-    fn new(file_name: String, rect: egui::Rect) -> Self {
-        Self {
-            filename: file_name,
-            rect,
-        }
-    }
-}
-
 struct MyApp {
     current_view: AppView,
     available_can_interfaces: Vec<String>,
@@ -402,7 +393,7 @@ impl MyApp {
                         ui.label(&plot_title);
                         ui.separator();
 
-                        let plot_response = Plot::new(plot_id)
+                        Plot::new(plot_id)
                             .legend(egui_plot::Legend::default())
                             .view_aspect(2.0)
                             .allow_scroll(false)
@@ -472,8 +463,19 @@ impl MyApp {
                         });
                         if ui.button("Start Reading").clicked() {
                             if let Ok(interval_ms) = self.modal_interval_str.parse::<u64>() {
+                                // Look up the data type from the EDS
+                                let data_type = self.sdo_data.as_ref()
+                                    .and_then(|sdo_map| sdo_map.get(&address.index))
+                                    .and_then(|sdo_object| sdo_object.sub_objects.get(&address.sub_index))
+                                    .and_then(|sub_object| SdoDataType::from_eds_type(&sub_object.data_type))
+                                    .unwrap_or(SdoDataType::Real32); // Fallback to Real32 if type unknown
+
                                 if let Some(tx) = &self.command_tx {
-                                    tx.send(Command::Subscribe { address: address.clone(), interval_ms }).unwrap();
+                                    tx.send(Command::Subscribe {
+                                        address: address.clone(),
+                                        interval_ms,
+                                        data_type,
+                                    }).unwrap();
                                 }
                                 self.subscriptions.insert(address.clone(), SdoSubscription {
                                     interval_ms,
diff --git a/mock-canopen-node/src/object_dictionary.rs b/mock-canopen-node/src/object_dictionary.rs
@@ -92,7 +92,7 @@ impl ObjectDictionary {
             0x01,
             || {
                 let mut rng = rand::rng();
-                let temp: f32 = rng.gen_range(20.0..30.0); // Random temperature between 20-30°C
+                let temp: f32 = rng.random_range(20.0..30.0); // Random temperature between 20-30°C
                 temp.to_le_bytes().to_vec()
             },
             SdoDataType::Real32,
@@ -104,7 +104,7 @@ impl ObjectDictionary {
             0x02,
             || {
                 let mut rng = rand::rng();
-                let pressure: f32 = rng.gen_range(95.0..105.0); // Random pressure 95-105 kPa
+                let pressure: f32 = rng.random_range(95.0..105.0); // Random pressure 95-105 kPa
                 pressure.to_le_bytes().to_vec()
             },
             SdoDataType::Real32,
@@ -132,7 +132,7 @@ impl ObjectDictionary {
             0x01,
             || {
                 let mut rng = rand::rng();
-                let voltage: f32 = rng.gen_range(11.5..12.5); // Random voltage 11.5-12.5V
+                let voltage: f32 = rng.random_range(11.5..12.5); // Random voltage 11.5-12.5V
                 voltage.to_le_bytes().to_vec()
             },
             SdoDataType::Real32,
@@ -144,7 +144,7 @@ impl ObjectDictionary {
             0x02,
             || {
                 let mut rng = rand::rng();
-                let current: f32 = rng.gen_range(0.5..5.0); // Random current 0.5-5.0A
+                let current: f32 = rng.random_range(0.5..5.0); // Random current 0.5-5.0A
                 current.to_le_bytes().to_vec()
             },
             SdoDataType::Real32,
@@ -162,7 +162,7 @@ impl ObjectDictionary {
             0x01,
             || {
                 let mut rng = rand::rng();
-                let rpm: i32 = rng.gen_range(1000..3000); // Random RPM 1000-3000
+                let rpm: i32 = rng.random_range(1000..3000); // Random RPM 1000-3000
                 rpm.to_le_bytes().to_vec()
             },
             SdoDataType::Int32,
diff --git a/mock-canopen-node/src/sdo_server.rs b/mock-canopen-node/src/sdo_server.rs
@@ -1,11 +1,11 @@
 //! SDO Server implementation for responding to SDO upload requests
 
 use socketcan::{CanFrame, StandardId, EmbeddedFrame};
-use canopen_common::{SdoDataType, SdoCommand};
+use canopen_common::SdoDataType;
 use crate::object_dictionary::ObjectDictionary;
 
 pub struct SdoServer {
-    node_id: u8,
+    _node_id: u8,  // Stored for potential future use (logging, multi-node support)
     object_dict: ObjectDictionary,
     request_cob_id: u16,  // 0x600 + node_id
     response_cob_id: u16, // 0x580 + node_id
@@ -14,7 +14,7 @@ pub struct SdoServer {
 impl SdoServer {
     pub fn new(node_id: u8, object_dict: ObjectDictionary) -> Self {
         Self {
-            node_id,
+            _node_id: node_id,
             object_dict,
             request_cob_id: 0x600 + node_id as u16,
             response_cob_id: 0x580 + node_id as u16,
