Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add lbs byte order support #29

Merged
merged 4 commits into from
Nov 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/frames/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,7 @@ impl<'a> TryFrom<&'a [u8]> for Frame<'a> {
if *data.last().ok_or(FrameError::LengthShort)? != 0x16 {
return Err(FrameError::InvalidStopByte);
}

let control_field = *data.get(4).ok_or(FrameError::LengthShort)?;

let address_field = *data.get(5).ok_or(FrameError::LengthShort)?;
match control_field {
0x53 => Ok(Frame::ControlFrame {
Expand Down
23 changes: 18 additions & 5 deletions src/user_data/data_information.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use super::data_information::{self};
use super::variable_user_data::DataRecordError;
use super::FixedDataHeader;

#[cfg_attr(feature = "serde", derive(serde::Serialize))]
#[derive(Debug, PartialEq)]
Expand Down Expand Up @@ -457,6 +458,7 @@ fn bcd_to_value_internal(
data: &[u8],
num_digits: usize,
sign: i32,
lsb_order: bool,
) -> Result<Data, DataRecordError> {
if data.len() < (num_digits + 1) / 2 {
return Err(DataRecordError::InsufficientData);
Expand All @@ -466,7 +468,12 @@ fn bcd_to_value_internal(
let mut current_weight = 1.0;

for i in 0..num_digits {
let byte = data.get(i / 2).ok_or(DataRecordError::InsufficientData)?;
let index = if lsb_order {
(num_digits - i - 1) / 2
} else {
i / 2
};
let byte = data.get(index).ok_or(DataRecordError::InsufficientData)?;

let digit = if i % 2 == 0 {
byte & 0x0F
Expand All @@ -487,14 +494,20 @@ fn bcd_to_value_internal(
}

impl DataFieldCoding {
pub fn parse<'a>(&self, input: &'a [u8]) -> Result<Data<'a>, DataRecordError> {
pub fn parse<'a>(
&self,
input: &'a [u8],
fixed_data_header: Option<&'a FixedDataHeader>,
) -> Result<Data<'a>, DataRecordError> {
let lsb_order = fixed_data_header.map(|x| x.lsb_order).unwrap_or(false);

macro_rules! bcd_to_value {
($data:expr, $num_digits:expr) => {{
bcd_to_value_internal($data, $num_digits, 1)
bcd_to_value_internal($data, $num_digits, 1, lsb_order)
}};

($data:expr, $num_digits:expr, $sign:expr) => {{
bcd_to_value_internal($data, $num_digits, $sign)
bcd_to_value_internal($data, $num_digits, $sign, lsb_order)
}};
}

Expand Down Expand Up @@ -875,7 +888,7 @@ mod tests {
#[test]
fn test_bcd_to_value_unsigned() {
let data = [0x54, 0x76, 0x98];
let result = bcd_to_value_internal(&data, 6, 1);
let result = bcd_to_value_internal(&data, 6, 1, false);
assert_eq!(
result.unwrap(),
Data {
Expand Down
77 changes: 49 additions & 28 deletions src/user_data/data_record.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use super::{
data_information::{Data, DataFieldCoding, DataInformation, DataInformationBlock, DataType},
value_information::{ValueInformation, ValueInformationBlock, ValueLabel},
variable_user_data::DataRecordError,
FixedDataHeader,
Copy link
Owner

@maebli maebli Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if we introduce DataRecordsMSB additionally instead of adding FixedDataHeader?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that should work as a frame can not contain mixed MSB/LSB. But in that case FixedDataHeader maybe should have a function "get_data_records" with returns DataRecords or DataRecordsMSB accordingly? Because with it it is on the user to check what MSB is the FixedDataHeader and construct the right DataRecords. Not a super big deal but easy to get wrong for someone new not aware of the MSB/LSB difference.
I am fine with splinting the ControlInformation into ResponseWithVariableDataStructureMSB and ResponseWithFixedDataStructureMSB, but will go over that change (and the empty line) once we have a plan for that here. Thanks for the quick review!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to get something working, I might just merge your PR and then create a new branch for refactoring. Can you also make PR with the other additions you have made on your main branch? :) Also can you PM me on discord if you have time, I'm interest to know how you are using the parser (purely intrest) and maybe we also discuss where the parser should be improved. Link to discord is on the main page

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I am fine with merging it as it is and then working from it.
I will also make a PR for all the other fixes I add on my main branch, I just wanted avoid to surprise your with like 5 different PRs :D

};
#[cfg_attr(feature = "serde", derive(serde::Serialize))]
#[derive(Debug, PartialEq)]
Expand Down Expand Up @@ -29,6 +30,44 @@ impl DataRecord<'_> {
}
}

impl<'a> DataRecord<'a> {
fn parse(
data: &'a [u8],
fixed_data_header: Option<&'a FixedDataHeader>,
) -> Result<Self, DataRecordError> {
let data_record_header = DataRecordHeader::try_from(data)?;
let mut offset = data_record_header
.raw_data_record_header
.data_information_block
.get_size();
let mut data_out = Data {
value: Some(DataType::ManufacturerSpecific(data)),
size: data.len(),
};
if let Some(x) = &data_record_header
.raw_data_record_header
.value_information_block
{
offset += x.get_size();
if let Some(data_info) = &data_record_header
.processed_data_record_header
.data_information
{
data_out = data_info.data_field_coding.parse(
data.get(offset..)
.ok_or(DataRecordError::InsufficientData)?,
fixed_data_header,
)?;
}
}

Ok(DataRecord {
data_record_header,
data: data_out,
})
}
}

#[cfg_attr(feature = "serde", derive(serde::Serialize))]
#[derive(Debug, PartialEq)]
pub struct DataRecordHeader<'a> {
Expand Down Expand Up @@ -121,37 +160,19 @@ impl<'a> TryFrom<&'a [u8]> for DataRecordHeader<'a> {
}
}

impl<'a> TryFrom<(&'a [u8], &'a FixedDataHeader)> for DataRecord<'a> {
type Error = DataRecordError;
fn try_from(
(data, fixed_data_header): (&'a [u8], &'a FixedDataHeader),
) -> Result<Self, Self::Error> {
Self::parse(data, Some(fixed_data_header))
}
}

impl<'a> TryFrom<&'a [u8]> for DataRecord<'a> {
type Error = DataRecordError;
fn try_from(data: &'a [u8]) -> Result<Self, Self::Error> {
let data_record_header = DataRecordHeader::try_from(data)?;
let mut offset = data_record_header
.raw_data_record_header
.data_information_block
.get_size();
let mut data_out = Data {
value: Some(DataType::ManufacturerSpecific(data)),
size: data.len(),
};
if let Some(x) = &data_record_header
.raw_data_record_header
.value_information_block
{
offset += x.get_size();
if let Some(data_info) = &data_record_header
.processed_data_record_header
.data_information
{
data_out = data_info.data_field_coding.parse(
data.get(offset..)
.ok_or(DataRecordError::InsufficientData)?,
)?;
}
}
Ok(DataRecord {
data_record_header,
data: data_out,
})
Self::parse(data, None)
}
}

Expand Down
119 changes: 106 additions & 13 deletions src/user_data/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ pub mod variable_user_data;
pub struct DataRecords<'a> {
offset: usize,
data: &'a [u8],
fixed_data_header: Option<&'a FixedDataHeader>,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that this inside the datarecords its part of a different OSI layer

}

#[cfg(feature = "serde")]
Expand Down Expand Up @@ -44,7 +45,11 @@ impl<'a> Iterator for DataRecords<'a> {
self.offset += 1;
}
_ => {
let record = DataRecord::try_from(self.data.get(self.offset..)?);
let record = if let Some(fixed_data_header) = self.fixed_data_header {
DataRecord::try_from((self.data.get(self.offset..)?, fixed_data_header))
} else {
DataRecord::try_from(self.data.get(self.offset..)?)
};
if let Ok(record) = record {
self.offset += record.get_size();
return Some(Ok(record));
Expand All @@ -60,8 +65,12 @@ impl<'a> Iterator for DataRecords<'a> {

impl<'a> DataRecords<'a> {
#[must_use]
pub const fn new(data: &'a [u8]) -> Self {
DataRecords { offset: 0, data }
pub const fn new(data: &'a [u8], fixed_data_header: Option<&'a FixedDataHeader>) -> Self {
DataRecords {
offset: 0,
data,
fixed_data_header,
}
}
}

Expand Down Expand Up @@ -146,7 +155,9 @@ impl From<ControlInformation> for Direction {
ControlInformation::HashProcedure(_) => Self::MasterToSlave,
ControlInformation::SendErrorStatus => Self::SlaveToMaster,
ControlInformation::SendAlarmStatus => Self::SlaveToMaster,
ControlInformation::ResponseWithVariableDataStructure => Self::SlaveToMaster,
ControlInformation::ResponseWithVariableDataStructure { lsb_order: _ } => {
Self::SlaveToMaster
}
ControlInformation::ResponseWithFixedDataStructure => Self::SlaveToMaster,
}
}
Expand Down Expand Up @@ -174,7 +185,7 @@ pub enum ControlInformation {
HashProcedure(u8),
SendErrorStatus,
SendAlarmStatus,
ResponseWithVariableDataStructure,
ResponseWithVariableDataStructure { lsb_order: bool },
ResponseWithFixedDataStructure,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add ResponseWithVariableDataStructureMSB instead of adding the enum info

}

Expand All @@ -201,7 +212,9 @@ impl ControlInformation {
0x90..=0x97 => Ok(Self::HashProcedure(byte - 0x90)),
0x70 => Ok(Self::SendErrorStatus),
0x71 => Ok(Self::SendAlarmStatus),
0x72 | 0x76 => Ok(Self::ResponseWithVariableDataStructure),
0x72 | 0x76 => Ok(Self::ResponseWithVariableDataStructure {
lsb_order: byte & 0x04 != 0,
}),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok now i understand, you should change this into something else:

0x72 = Ok(Self::ResponseWithVariableDataStructure),
0x76 = Ok(Self::ResponseWithVariableDataStructureMSB),

Same as https://github.com/rscada/libmbus/blob/cb28aca76e79d7852e3bf3c052c46f664d0fcdf5/mbus/mbus-protocol.h#L426

Also you could add the otehr MSB as well in this PR

https://github.com/rscada/libmbus/blob/cb28aca76e79d7852e3bf3c052c46f664d0fcdf5/mbus/mbus-protocol.h#L423C1-L423C50

0x73 | 0x77 => Ok(Self::ResponseWithFixedDataStructure),
_ => Err(ApplicationLayerError::InvalidControlInformation { byte }),
}
Expand Down Expand Up @@ -513,6 +526,7 @@ pub struct FixedDataHeader {
pub access_number: u8,
pub status: StatusField,
pub signature: u16,
pub lsb_order: bool,
Copy link
Owner

@maebli maebli Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not add this here, the information about MSB, needs to be injected differently somehow. Or are the other informations also needed in the other layer?

I want to seprate the layers as much as possible

}
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
#[derive(Debug, PartialEq)]
Expand Down Expand Up @@ -600,16 +614,23 @@ impl<'a> TryFrom<&'a [u8]> for UserDataBlock<'a> {
ControlInformation::HashProcedure(_) => todo!(),
ControlInformation::SendErrorStatus => todo!(),
ControlInformation::SendAlarmStatus => todo!(),
ControlInformation::ResponseWithVariableDataStructure => {
ControlInformation::ResponseWithVariableDataStructure { lsb_order } => {
let mut iter = data.iter().skip(1);
let mut identification_number_bytes = [
*iter.next().ok_or(ApplicationLayerError::InsufficientData)?,
*iter.next().ok_or(ApplicationLayerError::InsufficientData)?,
*iter.next().ok_or(ApplicationLayerError::InsufficientData)?,
*iter.next().ok_or(ApplicationLayerError::InsufficientData)?,
];
if lsb_order {
identification_number_bytes.reverse();
}

Ok(UserDataBlock::VariableDataStructure {
fixed_data_header: FixedDataHeader {
identification_number: IdentificationNumber::from_bcd_hex_digits([
*iter.next().ok_or(ApplicationLayerError::InsufficientData)?,
*iter.next().ok_or(ApplicationLayerError::InsufficientData)?,
*iter.next().ok_or(ApplicationLayerError::InsufficientData)?,
*iter.next().ok_or(ApplicationLayerError::InsufficientData)?,
])?,
identification_number: IdentificationNumber::from_bcd_hex_digits(
identification_number_bytes,
)?,
manufacturer: ManufacturerCode::from_id(u16::from_le_bytes([
*iter.next().ok_or(ApplicationLayerError::InsufficientData)?,
*iter.next().ok_or(ApplicationLayerError::InsufficientData)?,
Expand All @@ -629,6 +650,7 @@ impl<'a> TryFrom<&'a [u8]> for UserDataBlock<'a> {
*iter.next().ok_or(ApplicationLayerError::InsufficientData)?,
*iter.next().ok_or(ApplicationLayerError::InsufficientData)?,
]),
lsb_order,
},
variable_data_block: data
.get(13..data.len())
Expand Down Expand Up @@ -817,4 +839,75 @@ mod tests {
);
Ok(())
}

#[test]
fn test_lsb_frame() {
use crate::frames::Frame;
use crate::user_data::data_information::DataType;

let lsb_frame: &[u8] = &[
0x68, 0x64, 0x64, 0x68, 0x8, 0x7f, 0x76, 0x9, 0x67, 0x1, 0x6, 0x0, 0x0, 0x51, 0x4,
0x50, 0x0, 0x0, 0x0, 0x2, 0x6c, 0x38, 0x1c, 0xc, 0xf, 0x0, 0x80, 0x87, 0x32, 0x8c,
0x20, 0xf, 0x0, 0x0, 0x0, 0x0, 0xc, 0x14, 0x13, 0x32, 0x82, 0x58, 0xbc, 0x10, 0x15,
0x0, 0x25, 0x81, 0x25, 0x8c, 0x20, 0x13, 0x0, 0x0, 0x0, 0x0, 0x8c, 0x30, 0x13, 0x0,
0x0, 0x1, 0x61, 0x8c, 0x40, 0x13, 0x0, 0x0, 0x16, 0x88, 0xa, 0x3c, 0x1, 0x10, 0xa,
0x2d, 0x0, 0x80, 0xa, 0x5a, 0x7, 0x18, 0xa, 0x5e, 0x6, 0x53, 0xc, 0x22, 0x0, 0x16, 0x7,
0x26, 0x3c, 0x22, 0x0, 0x0, 0x33, 0x81, 0x4, 0x7e, 0x0, 0x0, 0x67, 0xc, 0xc, 0x16,
];
let non_lsb_frame: &[u8] = &[
0x68, 0xc7, 0xc7, 0x68, 0x8, 0x38, 0x72, 0x56, 0x73, 0x23, 0x72, 0x2d, 0x2c, 0x34, 0x4,
0x87, 0x0, 0x0, 0x0, 0x4, 0xf, 0x7f, 0x1c, 0x1, 0x0, 0x4, 0xff, 0x7, 0x8a, 0xad, 0x8,
0x0, 0x4, 0xff, 0x8, 0x6, 0xfe, 0x5, 0x0, 0x4, 0x14, 0x4e, 0x55, 0xb, 0x0, 0x84, 0x40,
0x14, 0x0, 0x0, 0x0, 0x0, 0x84, 0x80, 0x40, 0x14, 0x0, 0x0, 0x0, 0x0, 0x4, 0x22, 0x76,
0x7f, 0x0, 0x0, 0x34, 0x22, 0x8b, 0x2c, 0x0, 0x0, 0x2, 0x59, 0x61, 0x1b, 0x2, 0x5d,
0x5f, 0x10, 0x2, 0x61, 0x2, 0xb, 0x4, 0x2d, 0x55, 0x0, 0x0, 0x0, 0x14, 0x2d, 0x83, 0x0,
0x0, 0x0, 0x4, 0x3b, 0x6, 0x1, 0x0, 0x0, 0x14, 0x3b, 0xaa, 0x1, 0x0, 0x0, 0x4, 0xff,
0x22, 0x0, 0x0, 0x0, 0x0, 0x4, 0x6d, 0x6, 0x2c, 0x1f, 0x3a, 0x44, 0xf, 0xcf, 0x11, 0x1,
0x0, 0x44, 0xff, 0x7, 0xb, 0x69, 0x8, 0x0, 0x44, 0xff, 0x8, 0x54, 0xd3, 0x5, 0x0, 0x44,
0x14, 0x11, 0xf3, 0xa, 0x0, 0xc4, 0x40, 0x14, 0x0, 0x0, 0x0, 0x0, 0xc4, 0x80, 0x40,
0x14, 0x0, 0x0, 0x0, 0x0, 0x54, 0x2d, 0x3a, 0x0, 0x0, 0x0, 0x54, 0x3b, 0x28, 0x1, 0x0,
0x0, 0x42, 0x6c, 0x1, 0x3a, 0x2, 0xff, 0x1a, 0x1, 0x1a, 0xc, 0x78, 0x56, 0x73, 0x23,
0x72, 0x4, 0xff, 0x16, 0xe6, 0x84, 0x1e, 0x0, 0x4, 0xff, 0x17, 0xc1, 0xd5, 0xb4, 0x0,
0x12, 0x16,
];

let frames = [
(lsb_frame, 9670106, Some(DataType::Number(808732.0))),
(non_lsb_frame, 72237356, Some(DataType::Number(568714.0))),
];

for (frame, expected_iden_nr, data_record_value) in frames {
let frame = Frame::try_from(frame).unwrap();

if let Frame::LongFrame {
function: _,
address: _,
data,
} = frame
{
let user_data_block = UserDataBlock::try_from(data).unwrap();
if let UserDataBlock::VariableDataStructure {
fixed_data_header,
variable_data_block,
} = user_data_block
{
assert_eq!(
fixed_data_header.identification_number.number,
expected_iden_nr
);

let mut data_records =
DataRecords::try_from((variable_data_block, &fixed_data_header))
.unwrap()
.flatten();
data_records.next().unwrap();
assert_eq!(data_records.next().unwrap().data.value, data_record_value);
} else {
panic!("UserDataBlock is not a variable data structure");
}
} else {
panic!("Frame is not a long frame");
}
}
}
}
10 changes: 8 additions & 2 deletions src/user_data/variable_user_data.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use super::data_information::{self};
use super::DataRecords;
use super::{DataRecords, FixedDataHeader};

#[derive(Debug, PartialEq)]
pub enum DataRecordError {
Expand All @@ -20,7 +20,13 @@ impl From<DataRecordError> for VariableUserDataError {

impl<'a> From<&'a [u8]> for DataRecords<'a> {
fn from(data: &'a [u8]) -> Self {
DataRecords::new(data)
DataRecords::new(data, None)
}
}

impl<'a> From<(&'a [u8], &'a FixedDataHeader)> for DataRecords<'a> {
fn from((data, fixed_data_header): (&'a [u8], &'a FixedDataHeader)) -> Self {
DataRecords::new(data, Some(fixed_data_header))
}
}

Expand Down