Fix panic on disconnect() (#261)

uklotzde · web-flow · commit 8b04b2fa27ff · 2024-06-25T00:41:26.000+02:00
* Add debug assertions

* Fix panic on disconnect()

* Update changelog
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,7 +3,15 @@
 
 # Changelog
 
-## v0.13.0 (2024-06-23)
+## v0.13.1 (Unreleased)
+
+- Fix: Do not panic on disconnect.
+
+### Breaking Changes
+
+- Add `FunctionCode::Disconnect`.
+
+## v0.13.0 (2024-06-23, yanked)
 
 - Fix: Do not panic on mismatching request/response function codes.
 
@@ -12,7 +20,7 @@
 - Client: Replace `std::io::Error` with a custom error type that handles both
   protocol and transport errors.
 
-## v0.12.0 (2024-03-21)
+## v0.12.0 (2024-03-21, yanked)
 
 - Client: Support handling of _Modbus_ exceptions by using nested `Result`s.
 
diff --git a/examples/rtu-client.rs b/examples/rtu-client.rs
@@ -17,8 +17,11 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
 
     let mut ctx = rtu::attach_slave(port, slave);
     println!("Reading a sensor value");
-    let rsp = ctx.read_holding_registers(0x082B, 2).await?;
+    let rsp = ctx.read_holding_registers(0x082B, 2).await??;
     println!("Sensor value is: {rsp:?}");
 
+    println!("Disconnecting");
+    ctx.disconnect().await??;
+
     Ok(())
 }
diff --git a/examples/tcp-client.rs b/examples/tcp-client.rs
@@ -22,5 +22,8 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
     let id = String::from_utf8(bytes).unwrap();
     println!("The coupler ID is '{id}'");
 
+    println!("Disconnecting");
+    ctx.disconnect().await??;
+
     Ok(())
 }
diff --git a/src/codec/rtu.rs b/src/codec/rtu.rs
@@ -329,7 +329,12 @@ impl<'a> Encoder<RequestAdu<'a>> for ClientCodec {
     type Error = Error;
 
     fn encode(&mut self, adu: RequestAdu<'a>, buf: &mut BytesMut) -> Result<()> {
-        if adu.disconnect {
+        let RequestAdu {
+            hdr,
+            pdu,
+            disconnect,
+        } = adu;
+        if disconnect {
             // The disconnect happens implicitly after letting this request
             // fail by returning an error. This will drop the attached
             // transport, e.g. for closing a stale, exclusive connection
@@ -339,7 +344,6 @@ impl<'a> Encoder<RequestAdu<'a>> for ClientCodec {
                 "Disconnecting - not an error",
             ));
         }
-        let RequestAdu { hdr, pdu, .. } = adu;
         let pdu_data: Bytes = pdu.try_into()?;
         buf.reserve(pdu_data.len() + 3);
         buf.put_u8(hdr.slave_id);
diff --git a/src/codec/tcp.rs b/src/codec/tcp.rs
@@ -137,7 +137,12 @@ impl<'a> Encoder<RequestAdu<'a>> for ClientCodec {
     type Error = Error;
 
     fn encode(&mut self, adu: RequestAdu<'a>, buf: &mut BytesMut) -> Result<()> {
-        if adu.disconnect {
+        let RequestAdu {
+            hdr,
+            pdu,
+            disconnect,
+        } = adu;
+        if disconnect {
             // The disconnect happens implicitly after letting this request
             // fail by returning an error. This will drop the attached
             // transport, e.g. for terminating an open connection.
@@ -146,7 +151,6 @@ impl<'a> Encoder<RequestAdu<'a>> for ClientCodec {
                 "Disconnecting - not an error",
             ));
         }
-        let RequestAdu { hdr, pdu, .. } = adu;
         let pdu_data: Bytes = pdu.try_into()?;
         buf.reserve(pdu_data.len() + 7);
         buf.put_u16(hdr.transaction_id);
diff --git a/src/frame/mod.rs b/src/frame/mod.rs
@@ -46,42 +46,49 @@ pub enum FunctionCode {
 
     /// Custom Modbus Function Code.
     Custom(u8),
+
+    Disconnect,
 }
 
 impl FunctionCode {
     /// Create a new [`FunctionCode`] with `value`.
     #[must_use]
     pub const fn new(value: u8) -> Self {
         match value {
-            0x01 => FunctionCode::ReadCoils,
-            0x02 => FunctionCode::ReadDiscreteInputs,
-            0x05 => FunctionCode::WriteSingleCoil,
-            0x06 => FunctionCode::WriteSingleRegister,
-            0x03 => FunctionCode::ReadHoldingRegisters,
-            0x04 => FunctionCode::ReadInputRegisters,
-            0x0F => FunctionCode::WriteMultipleCoils,
-            0x10 => FunctionCode::WriteMultipleRegisters,
-            0x16 => FunctionCode::MaskWriteRegister,
-            0x17 => FunctionCode::ReadWriteMultipleRegisters,
-            code => FunctionCode::Custom(code),
+            0x01 => Self::ReadCoils,
+            0x02 => Self::ReadDiscreteInputs,
+            0x05 => Self::WriteSingleCoil,
+            0x06 => Self::WriteSingleRegister,
+            0x03 => Self::ReadHoldingRegisters,
+            0x04 => Self::ReadInputRegisters,
+            0x0F => Self::WriteMultipleCoils,
+            0x10 => Self::WriteMultipleRegisters,
+            0x16 => Self::MaskWriteRegister,
+            0x17 => Self::ReadWriteMultipleRegisters,
+            code => Self::Custom(code),
         }
     }
 
     /// Get the [`u8`] value of the current [`FunctionCode`].
+    ///
+    /// # Panics
+    ///
+    /// Panics on [`Disconnect`](Self::Disconnect) which has no corresponding Modbus function code.
     #[must_use]
     pub const fn value(self) -> u8 {
         match self {
-            FunctionCode::ReadCoils => 0x01,
-            FunctionCode::ReadDiscreteInputs => 0x02,
-            FunctionCode::WriteSingleCoil => 0x05,
-            FunctionCode::WriteSingleRegister => 0x06,
-            FunctionCode::ReadHoldingRegisters => 0x03,
-            FunctionCode::ReadInputRegisters => 0x04,
-            FunctionCode::WriteMultipleCoils => 0x0F,
-            FunctionCode::WriteMultipleRegisters => 0x10,
-            FunctionCode::MaskWriteRegister => 0x16,
-            FunctionCode::ReadWriteMultipleRegisters => 0x17,
-            FunctionCode::Custom(code) => code,
+            Self::ReadCoils => 0x01,
+            Self::ReadDiscreteInputs => 0x02,
+            Self::WriteSingleCoil => 0x05,
+            Self::WriteSingleRegister => 0x06,
+            Self::ReadHoldingRegisters => 0x03,
+            Self::ReadInputRegisters => 0x04,
+            Self::WriteMultipleCoils => 0x0F,
+            Self::WriteMultipleRegisters => 0x10,
+            Self::MaskWriteRegister => 0x16,
+            Self::ReadWriteMultipleRegisters => 0x17,
+            Self::Custom(code) => code,
+            Self::Disconnect => unreachable!(),
         }
     }
 }
diff --git a/src/service/rtu.rs b/src/service/rtu.rs
@@ -48,27 +48,36 @@ where
     }
 
     async fn call(&mut self, req: Request<'_>) -> Result<Response> {
-        let disconnect = req == Request::Disconnect;
-        let req_function_code = req.function_code();
-        let req_adu = self.next_request_adu(req, disconnect);
+        let req_function_code = if matches!(req, Request::Disconnect) {
+            None
+        } else {
+            Some(req.function_code())
+        };
+        let req_adu = self.next_request_adu(req, req_function_code.is_none());
         let req_hdr = req_adu.hdr;
 
         self.framed.read_buffer_mut().clear();
-
         self.framed.send(req_adu).await?;
+
         let res_adu = self
             .framed
             .next()
             .await
             .unwrap_or_else(|| Err(io::Error::from(io::ErrorKind::BrokenPipe)))?;
-
-        let ResponsePdu(result) = res_adu.pdu;
+        let ResponseAdu {
+            hdr: res_hdr,
+            pdu: res_pdu,
+        } = res_adu;
+        let ResponsePdu(result) = res_pdu;
 
         // Match headers of request and response.
-        if let Err(message) = verify_response_header(&req_hdr, &res_adu.hdr) {
+        if let Err(message) = verify_response_header(&req_hdr, &res_hdr) {
             return Err(ProtocolError::HeaderMismatch { message, result }.into());
         }
 
+        debug_assert!(req_function_code.is_some());
+        let req_function_code = req_function_code.unwrap();
+
         // Match function codes of request and response.
         let rsp_function_code = match &result {
             Ok(response) => response.function_code(),
diff --git a/src/service/tcp.rs b/src/service/tcp.rs
@@ -71,27 +71,36 @@ where
 
     pub(crate) async fn call(&mut self, req: Request<'_>) -> Result<Response> {
         log::debug!("Call {:?}", req);
-        let disconnect = req == Request::Disconnect;
-        let req_function_code = req.function_code();
-        let req_adu = self.next_request_adu(req, disconnect);
+        let req_function_code = if matches!(req, Request::Disconnect) {
+            None
+        } else {
+            Some(req.function_code())
+        };
+        let req_adu = self.next_request_adu(req, req_function_code.is_none());
         let req_hdr = req_adu.hdr;
 
         self.framed.read_buffer_mut().clear();
-
         self.framed.send(req_adu).await?;
+
         let res_adu = self
             .framed
             .next()
             .await
             .ok_or_else(io::Error::last_os_error)??;
-
-        let ResponsePdu(result) = res_adu.pdu;
+        let ResponseAdu {
+            hdr: res_hdr,
+            pdu: res_pdu,
+        } = res_adu;
+        let ResponsePdu(result) = res_pdu;
 
         // Match headers of request and response.
-        if let Err(message) = verify_response_header(&req_hdr, &res_adu.hdr) {
+        if let Err(message) = verify_response_header(&req_hdr, &res_hdr) {
             return Err(ProtocolError::HeaderMismatch { message, result }.into());
         }
 
+        debug_assert!(req_function_code.is_some());
+        let req_function_code = req_function_code.unwrap();
+
         // Match function codes of request and response.
         let rsp_function_code = match &result {
             Ok(response) => response.function_code(),

Original file line number	Diff line number	Diff line change
`@@ -22,5 +22,8 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {`
`22`	`22`	`let id = String::from_utf8(bytes).unwrap();`
`23`	`23`	`println!("The coupler ID is '{id}'");`
`24`	`24`
	`25`	`+ println!("Disconnecting");`
	`26`	`+ ctx.disconnect().await??;`
	`27`	`+`
`25`	`28`	`Ok(())`
`26`	`29`	`}`