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

mbus_parse_hex -n ignores parsing problem #169

Open
fredrik-sk opened this issue Jun 30, 2020 · 4 comments
Open

mbus_parse_hex -n ignores parsing problem #169

fredrik-sk opened this issue Jun 30, 2020 · 4 comments

Comments

@fredrik-sk
Copy link
Contributor

PR #168 executes the code for normalized output, which probably has not been executed so much until now.

As found here #168 (comment)
running mbus_hex_parse -n sen_pollutherm.hex causes some errors to be printed but the program returns with exitcode 0.

Adding an else statment like this

--- a/mbus/mbus-protocol-aux.c
+++ b/mbus/mbus-protocol-aux.c
@@ -1455,6 +1455,8 @@ mbus_data_variable_xml_normalized(mbus_data_variable *data)
             }
             else
             {
+                free(buff);
+                return NULL;
             }

             len += snprintf(&buff[len], buff_size - len, "    </DataRecord>\n\n");

makes the test output this:

mbus_vif_unit_normalize: Unknown VIF 0x07B
mbus_vib_unit_normalize: Error mbus_vif_unit_normalize
mbus_parse_variable_record: problem with mbus_vib_unit_normalize
Failed to generate XML representation of MBUS frame:
Unable to generate XML for test/test-frames/sen_pollutherm.hex
fredrik-sk added a commit to fredrik-sk/libmbus that referenced this issue Jun 30, 2020
Update mbus_parse_variable_record so it return NULL if
mbus_parse_variable_record return NULL.
This makes the program mbus_parse_hex exit with non zero exitcode.
@fredrik-sk
Copy link
Contributor Author

Can someone that has access to the specification check if VIF=0x7B is allowed?

According to https://m-bus.com/documentation-wired/08-appendix, I cannot se anything that indicates it.
Or are we off when decoding the data?

sen_pollutherm.hex after some re-formating

68 42 42 68 08 08 
72 76 00 05 21 18 4E 31 04 51 00 00 00
0C 07 64 08 00 00
0C 14 92 98 79 00 
0C 7B 02 03 00 00  # <==
0C 2C 58 54 00 00 
0A 5A 55 07 0A 5E 94 05 0B 60 76 60 01 0C 78 76 00 05 21 0C FD 10 76 00 05 21 1F B3 16

@lategoodbye
Copy link
Collaborator

I prefer to fix the decoding issue instead of disabling the complete XML output.

@fredrik-sk
Copy link
Contributor Author

I prefer to fix the decoding issue instead of disabling the complete XML output.

@lategoodbye : Yes, fixing a decoding error is something we ALSO should do, but according to me, that is a separate issue.

The suggested update in #170, 7297899, follows the rule "fail early, fail hard" to make sure that problems are noticed and not ignored.

This change will make a whole class of potential problems detectable when running tests.

@lategoodbye
Copy link
Collaborator

I've a different opinion about this. There will always a byte the libmbus cannot decode and the public available document won't save us everytime. From a unit test point of view the suggested "fail early, fail hard" is good, but from end user perspective i'm concerned that this could be seen as a regression and the users tend to use older versions.

So i prefer to keep the best effort XML output in error case, but make mbus_parse_hex fail with an error code.

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

2 participants