From 8b04b2fa27ff4103b40893c1dee4da12c1867094 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 25 Jun 2024 00:41:26 +0200 Subject: [PATCH] Fix panic on disconnect() (#261) * Add debug assertions * Fix panic on disconnect() * Update changelog --- CHANGELOG.md | 12 ++++++++-- examples/rtu-client.rs | 5 ++++- examples/tcp-client.rs | 3 +++ src/codec/rtu.rs | 8 +++++-- src/codec/tcp.rs | 8 +++++-- src/frame/mod.rs | 51 ++++++++++++++++++++++++------------------ src/service/rtu.rs | 23 +++++++++++++------ src/service/tcp.rs | 23 +++++++++++++------ 8 files changed, 90 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a267d2c..27d18c7a 100644 --- 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 index 3d592c93..80ffe062 100644 --- a/examples/rtu-client.rs +++ b/examples/rtu-client.rs @@ -17,8 +17,11 @@ async fn main() -> Result<(), Box> { 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 index 36e06bbb..48783355 100644 --- a/examples/tcp-client.rs +++ b/examples/tcp-client.rs @@ -22,5 +22,8 @@ async fn main() -> Result<(), Box> { 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 index 51df1154..adb101d5 100644 --- a/src/codec/rtu.rs +++ b/src/codec/rtu.rs @@ -329,7 +329,12 @@ impl<'a> Encoder> 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> 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 index 0d127bc9..168dfb85 100644 --- a/src/codec/tcp.rs +++ b/src/codec/tcp.rs @@ -137,7 +137,12 @@ impl<'a> Encoder> 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> 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 index ddf2c9d2..39682ad1 100644 --- a/src/frame/mod.rs +++ b/src/frame/mod.rs @@ -46,6 +46,8 @@ pub enum FunctionCode { /// Custom Modbus Function Code. Custom(u8), + + Disconnect, } impl FunctionCode { @@ -53,35 +55,40 @@ impl FunctionCode { #[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 index 239c3792..5e91c03e 100644 --- a/src/service/rtu.rs +++ b/src/service/rtu.rs @@ -48,27 +48,36 @@ where } async fn call(&mut self, req: Request<'_>) -> Result { - 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 index 19348865..a61f07b7 100644 --- 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 { 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(),