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

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

Closed
fredrik-sk opened this issue Jun 5, 2020 · 6 comments
Closed

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

fredrik-sk opened this issue Jun 5, 2020 · 6 comments

Comments

@fredrik-sk
Copy link
Contributor

fredrik-sk commented Jun 5, 2020

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
Copy link
Contributor Author

Update: Some more research showed that this was introduced by commit 2f9fa5c, 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.

2f9fa5c#r39755040

@lategoodbye
Copy link
Collaborator

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

@Apollon77
Copy link
Contributor

Apollon77 commented Jun 30, 2020

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
Copy link
Collaborator

@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
Copy link
Contributor

@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
Copy link
Collaborator

I consider this as fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants