rscada / libmbus

Meter-bus library and utility programs
http://www.rscada.se/libmbus
BSD 3-Clause "New" or "Revised" License
217 stars 137 forks source link

Running the tests on unmodified master gives printouts (diffs) #163

Closed fredrik-sk closed 4 years ago

fredrik-sk commented 4 years ago

I created a Dockerfile to run the tests, and ran the tests on an unmodified master. And I got a bunch of printouts (diff).

https://github.com/fredrik-sk/libmbus/runs/742450655?check_suite_focus=true

EDIT: Here is a copy of the printouts.

--- test/test-frames/electricity-meter-2.xml    2020-06-05 13:47:48.073790560 +0000
+++ test/test-frames/electricity-meter-2.xml.new    2020-06-05 13:47:51.117813164 +0000
@@ -2,7 +2,7 @@
 <MBusData>

     <SlaveInformation>
-        <Id>5000345</Id>
+        <Id>5000205</Id>
         <Manufacturer>@@@</Manufacturer>
         <Version>18</Version>
         <ProductName></ProductName>

mbus_parse: Invalid M-Bus frame length.
Unable to generate XML for test/test-frames/invalid_length.hex
mbus_frame_data_parse: Invalid length for fixed data.
Unable to generate XML for test/test-frames/invalid_length2.hex
--- test/test-frames/landis+gyr_ultraheat_t230.xml  2020-06-05 13:47:48.077790590 +0000
+++ test/test-frames/landis+gyr_ultraheat_t230.xml.new  2020-06-05 13:47:51.225813961 +0000
@@ -72,7 +72,7 @@
         <Function>Instantaneous value</Function>
         <StorageNumber>0</StorageNumber>
         <Unit>Temperature Difference (1e-1  deg C)</Unit>
-        <Value>1500002</Value>
+        <Value>-2</Value>
     </DataRecord>

     <DataRecord id="9">
fredrik-sk commented 4 years ago

Update: Some more research showed that this was introduced by commit 2f9fa5ccc8aa032556ab77b7cafcb24670b6bf8d, the code was updated but not the tests. Reverting that commit makes the test run clean again.

I will look into this some more.

Added a comment: According to wikipedia ( https://en.wikipedia.org/wiki/Binary-coded_decimal , section Packed BCD), 0xF0 should be interpreted as a positve number.

https://github.com/rscada/libmbus/commit/2f9fa5ccc8aa032556ab77b7cafcb24670b6bf8d#r39755040

lategoodbye commented 4 years ago

I think, i've found a solution. Unfortunately we have a regression in #151 which must be fixed first.

Apollon77 commented 4 years ago

I also have such a case ... I brought my branch of libmbus which is used to buil "node-mbus" to the current status of the master but my Node-mbs tests fail because also here I have one case where the "SlaveInformation.Id" field changes.

I already have the "improved BCD decoding" commit in too!

My Test use the following hex: 689292680801723E020005434C1202130000008C1004521200008C1104521200008C2004334477018C21043344770102FDC9FF01ED0002FDDBFF01200002ACFF014F008240ACFF01EEFF02FDC9FF02E70002FDDBFF02230002ACFF0251008240ACFF02F1FF02FDC9FF03E40002FDDBFF03450002ACFF03A0008240ACFF03E0FF02FF68000002ACFF0040018240ACFF00BFFF01FF1304D916

And here link to test error https://travis-ci.org/github/Apollon77/node-mbus/jobs/703332935#L1114

Branch of libmbus is https://github.com/Apollon77/libmbus/commits/build-windows

lategoodbye commented 4 years ago

@Apollon77 Your hex contains 3E020005 which is actually slave id 500023E. I'm not sure it's allowed to have hex digits in a slave id and i don't know this comes from a real device. But make libmbus hiding this issue to make your tests succeed doesn't make sense to me.

So please describe your expectations.

Apollon77 commented 4 years ago

@lategoodbye if it is that way I'm fine ... then this change is "simply because decoding was broken before" and so it is fixed now :-) I was just wondering which is right ... When I remember correctly I had that "data string" from a real device. Then I fix the test expectation!

lategoodbye commented 4 years ago

I consider this as fixed