feat: add lbs byte order support #29

dragonnn · 2024-11-12T09:40:48Z

Some heatmeters use lsb byter order instead of msb. This affects how BCD should be decoded while reading identification number and any data records that are marked as BCD.
The bit indicating that the frame is lsb or msb is inside the control information part of the frame.
The test needs #27 to be able to pass.
The DataRecord api for decoding with lsb support isn't the best because it takes a tuple containing the record bytes and FixedDataHeader, and to not break any previous code I did keep it to be able to take the record bytes only. I think in the future a struct that handles parsing the whole frame and handles such cases for the user might be preferable as now it is easy to forget to pass the FixedDataHeader and fallback to decode without checking for lsb byte order.

maebli · 2024-11-19T22:11:19Z

Hey, thanks for the PR! This is is how I understand the state of things: you would like BCD to be revesed in byte order.

My question are:

(1) Does this go against the MBUS norm? I think yes. Therefore I propose this be done with a "feature" rather than a run time boolean
(2) Why do you not just do the change at the very end, when you create a table / json / yaml? You could reverse the byte order at the very end. What do you think?

dragonnn · 2024-11-20T09:06:25Z

This isn't simple bcd in reversed in byte order. It uses a bit in C Field (Control Field) to find out if the frame provided is coded in LSB or MSB (https://github.com/maebli/m-bus-parser/pull/29/files#diff-cd9539fdb6e41539f62bd90927ed31026413e7c20f5ecf794429cd1c6a33e5cbR215), as far I know this is part of the MBUS standard
pymeterbus has similar code for handling that https://github.com/ganehag/pyMeterBus/blob/f0d6ae22f3943a2f7e1e7cc58bb9a615dbc98ffc/meterbus/telegram_body.py#L228 (where I took the information how to handle it right).
Putting it behind a feature will not work for me at all because the same code needs to handle right MSB and LSB frames as my project is parsing a lot frames (we have systems that read a few hundred meters from a bunch of different manufacturers).
Doing it on my side would require basically re-implementing of getting the C Field out of the raw frame, checking for the byte and then going through already parsed values and then reversing the numbers :/, that would be really ugly

maebli · 2024-11-20T12:38:41Z

ok thanks for the feedback, i feel like I have not understood it properly. I will need some more time. Is this ok for you? By when do you need it?

dragonnn · 2024-11-20T12:50:27Z

No problem, take your time. I am using my fork in the mean time anyway, I just would like to merge it at some point so we have a single create that is as good as it can be instead of having a few forks where each one has different stuff fixed :).

maebli

I have done a first part of the review, not finished entirly. Maybe you can give me a feedback. I think we need to find a better way to send the LSB information to right place from the C Field detection to the actual application. I will think about it and come back.

maebli · 2024-11-20T19:54:47Z

src/frames/mod.rs

@@ -162,6 +162,7 @@ impl<'a> TryFrom<&'a [u8]> for Frame<'a> {
                let control_field = *data.get(4).ok_or(FrameError::LengthShort)?;

                let address_field = *data.get(5).ok_or(FrameError::LengthShort)?;
+


I would revert this change, as you have not touched this file otherwise. I think its because you had printlns here for debugging that you removed.

maebli · 2024-11-20T20:48:20Z

src/user_data/mod.rs

-            0x72 | 0x76 => Ok(Self::ResponseWithVariableDataStructure),
+            0x72 | 0x76 => Ok(Self::ResponseWithVariableDataStructure {
+                lsb_order: byte & 0x04 != 0,
+            }),


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

maebli · 2024-11-20T20:48:49Z

src/user_data/mod.rs

@@ -174,7 +185,7 @@ pub enum ControlInformation {
    HashProcedure(u8),
    SendErrorStatus,
    SendAlarmStatus,
-    ResponseWithVariableDataStructure,
+    ResponseWithVariableDataStructure { lsb_order: bool },
    ResponseWithFixedDataStructure,


add ResponseWithVariableDataStructureMSB instead of adding the enum info

maebli · 2024-11-20T20:55:06Z

src/user_data/mod.rs

@@ -17,6 +17,7 @@ pub mod variable_user_data;
 pub struct DataRecords<'a> {
    offset: usize,
    data: &'a [u8],
+    fixed_data_header: Option<&'a FixedDataHeader>,


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

maebli · 2024-11-20T20:57:29Z

src/user_data/mod.rs

@@ -513,6 +526,7 @@ pub struct FixedDataHeader {
    pub access_number: u8,
    pub status: StatusField,
    pub signature: u16,
+    pub lsb_order: bool,


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

maebli · 2024-11-20T21:07:09Z

src/user_data/data_record.rs

@@ -2,6 +2,7 @@ use super::{
    data_information::{Data, DataFieldCoding, DataInformation, DataInformationBlock, DataType},
    value_information::{ValueInformation, ValueInformationBlock, ValueLabel},
    variable_user_data::DataRecordError,
+    FixedDataHeader,


what if we introduce DataRecordsMSB additionally instead of adding FixedDataHeader?

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!

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

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

dragonnn added 3 commits November 12, 2024 10:32

feat: add lsb decoding to identification_number parsing

68ddbac

feat: add lsb order decoding in data records

b889905

cleanup debug printing

2086105

maebli requested changes Nov 20, 2024

View reviewed changes

remove white space

c88a50a

maebli approved these changes Nov 24, 2024

View reviewed changes

maebli merged commit 864faf5 into maebli:main Nov 24, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add lbs byte order support #29

feat: add lbs byte order support #29

dragonnn commented Nov 12, 2024

maebli commented Nov 19, 2024

dragonnn commented Nov 20, 2024

maebli commented Nov 20, 2024

dragonnn commented Nov 20, 2024

maebli left a comment

maebli Nov 20, 2024

maebli Nov 20, 2024

maebli Nov 20, 2024

maebli Nov 20, 2024

maebli Nov 20, 2024 •

edited

Loading

maebli Nov 20, 2024 •

edited

Loading

dragonnn Nov 21, 2024

maebli Nov 22, 2024

dragonnn Nov 22, 2024

		@@ -162,6 +162,7 @@ impl<'a> TryFrom<&'a [u8]> for Frame<'a> {
		let control_field = *data.get(4).ok_or(FrameError::LengthShort)?;

		let address_field = *data.get(5).ok_or(FrameError::LengthShort)?;

feat: add lbs byte order support #29

feat: add lbs byte order support #29

Conversation

dragonnn commented Nov 12, 2024

maebli commented Nov 19, 2024

dragonnn commented Nov 20, 2024

maebli commented Nov 20, 2024

dragonnn commented Nov 20, 2024

maebli left a comment

Choose a reason for hiding this comment

maebli Nov 20, 2024

Choose a reason for hiding this comment

maebli Nov 20, 2024

Choose a reason for hiding this comment

maebli Nov 20, 2024

Choose a reason for hiding this comment

maebli Nov 20, 2024

Choose a reason for hiding this comment

maebli Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

maebli Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

dragonnn Nov 21, 2024

Choose a reason for hiding this comment

maebli Nov 22, 2024

Choose a reason for hiding this comment

dragonnn Nov 22, 2024

Choose a reason for hiding this comment

maebli Nov 20, 2024 •

edited

Loading

maebli Nov 20, 2024 •

edited

Loading