hirschenberger / modbus-rs

Modbus implementation in pure Rust
MIT License
89 stars 22 forks source link

Proposal: Functions for dealing with multi-register values #33

Open groffta opened 2 years ago

groffta commented 2 years ago

Commonly, Modbus utilizes values that are represented by multiple registers (IEEE 32-bit Float being the most common). Would there be any interest in including this higher-level functionality? If so, what thoughts are there on how to implement this? I've accomplished this in my own projects by implementing traits on f32 and u32 that provide T::to_registers(self) -> [u16;2] and T::from_registers([u16;2]) -> T. For a more general implementation, there would need to be a way to specify either low or high word order. Any input here is appreciated.

hirschenberger commented 2 years ago

I also thought about a more descriptive and high-level interface. Dreaming of a generically parameterized trait that statically describe the process image. In C++ this would be possible using some template-metaprograming. Rust is still a little limited in that regard and tbh. I'm also no Rust guru (but maybe a good opportunity to learn). I think the biggest drawback is that rust doesn't have variadic generics so maybe some macro-magic is required here.

The goal would be to let the compiler validate data-access at compiletime to find type and index errors and to access a static stack array efficiently.

groffta commented 2 years ago

I'm not sure you would need that level of abstraction in the modbus library. It feels like that level of validation should be left to device driver implementations. That being said, you could add the following type agnostic functions as a QoL improvement:

Transport::read_holding_value<T: ModbusValue>(&mut self, start_addr: u16) -> Result<T>;
Transport::read_input_value<T: ModbusValue>(&mut self, start_addr: u16) -> Result<T>;
Transport::write_value<T: ModbusValue>(&mut self, start_addr: u16, val: T) -> Result<()>;

pub trait ModbusValue {
  fn to_registers(self) -> Vec<u16>;
  fn from_registers(registers: Vec<u16>) -> Option<Self>;
  fn word_count() -> usize;
}

impl ModbusValue for u8 { ... }
impl ModbusValue for u16 { ... }
impl ModbusValue for u32 { ... }
impl ModbusValue for u64 { ... }
impl ModbusValue for i8 { ... }
impl ModbusValue for i16 { ... }
impl ModbusValue for i32 { ... }
impl ModbusValue for i64 { ... }
impl ModbusValue for f32 { ... }
impl ModbusValue for f64 { ... }

And add the following to deal with word order:

pub enum WordOrder {
  LowHigh,
  HighLow,
}

Transport::get_word_order(&self) -> WordOrder;
Transport::set_word_order(&mut self, order: WordOrder);

I'm willing to implement this if you don't have any objections. What are your thoughts?

hirschenberger commented 2 years ago

LGTM

Would it make sense to also add Coils to ModbusValue or even implement ModbusValue for Vec<Coil>?

groffta commented 2 years ago

On the surface, it looks like Coils should be included, but since they work on the bit level, and have asymmetric data representation depending on what function code you are using, I think it makes more sense to leave them as their own explicit functions.