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.

Sign up for GitHub

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

Jump to bottom

Improve BCD decoding #167

Merged
merged 2 commits into from
Jun 28, 2020
Merged

Improve BCD decoding #167

merged 2 commits into from
Jun 28, 2020

Conversation

lategoodbye
Copy link
Collaborator

This pull request tries to solve the issue #163

At the end the whole BCD decoding is depending on the context. So add a new function which preserve all the BCD information.

@lategoodbye
Copy link
Collaborator Author

@fredrik-sk are you fine with this compromise?

fredrik-sk
fredrik-sk reviewed Jun 27, 2020
View reviewed changes
mbus/mbus-protocol.c
@@ -4015,7 +4040,7 @@ mbus_data_variable_header_xml(mbus_data_variable_header *header)
{
len += snprintf(&buff[len], sizeof(buff) - len, " <SlaveInformation>\n");

len += snprintf(&buff[len], sizeof(buff) - len, " <Id>%lld</Id>\n", mbus_data_bcd_decode(header->id_bcd, 4));
len += snprintf(&buff[len], sizeof(buff) - len, " <Id>%llX</Id>\n", mbus_data_bcd_decode_hex(header->id_bcd, 4));
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and above this line you use "%llX", (upper case hexdecimal). But lines 4221 and below use "%llx" (lower case hexadecimal).
Is this intentional, or is there a correct way that should be used all through the code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, thanks for noticing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be fixed now

@lategoodbye lategoodbye force-pushed the improve-bcd-decoding branch from 9614634 to 280fc80 Compare June 27, 2020 16:25
fredrik-sk
fredrik-sk reviewed Jun 27, 2020
View reviewed changes
test/test-frames/ELS_Elster-F96-Plus.xml
@@ -48,14 +48,14 @@
<Function>Value during error state</Function>
<StorageNumber>0</StorageNumber>
<Unit>Power (W)</Unit>
<Value>144445223</Value>
<Value>DDDDEBBD</Value>
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a hexadecimal number. Should this be using some other encoding?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the intention of this patch. Since this is a error value (look at the function), it's better to signalize this with a hex value than a unrealistic decimal.

@fredrik-sk
Copy link
Contributor

@fredrik-sk are you fine with this compromise?

I'm glad that something happends. And that you have found and corrected some issue.

lategoodbye and others added 2 commits June 28, 2020 10:18
@lategoodbye
Introduce mbus_data_bcd_decode_hex
bd55182 
The convert function mbus_data_bcd_decode (BCD to decimal) suffers from
information loss in case of hexacimal digits. So introduce a new function
mbus_data_bcd_decode_hex (BCD to hexadecimal), which isn't affected and
use this for default XML output. But keep mbus_data_bcd_decode for
normalized output.
@lategoodbye
Update XML reference output
3d41518 
This updates the XML reference output after recent decoding changes.
@lategoodbye lategoodbye force-pushed the improve-bcd-decoding branch from 280fc80 to 3d41518 Compare June 28, 2020 08:21
@lategoodbye lategoodbye merged commit 17a7328 into master Jun 28, 2020
@lategoodbye
Copy link
Collaborator Author

Thanks for the review

@fredrik-sk
Copy link
Contributor

I had one more comment. I see the change has been merged.
Should we return both the raw value and the decoded?

@lategoodbye
Copy link
Collaborator Author

Should we return both the raw value and the decoded?

Not sure that i understand your question. Do you mean decimal + hex?

@fredrik-sk
Copy link
Contributor

Yes. Since in the case with negative BCD, seing a minus makes more sense then an F, and having to apply the algorithm by yourself.
My first approach was calling it "raw value", and "decoded value".

@lategoodbye
Copy link
Collaborator Author

I like to avoid such a structure change in the XML structure. Maybe you prefer the normalized output. Let's stay at this example (SLB_CF-Compact-Integral-MK-MaXX).

default output:

   <DataRecord id="6">
        <Function>Instantaneous value</Function>
        <StorageNumber>0</StorageNumber>
        <Unit>Temperature Difference (1e-2  deg C)</Unit>
        <Value>F00018</Value>
    </DataRecord>

normalized output:

<DataRecord id="6">
        <Function>Instantaneous value</Function>
        <StorageNumber>0</StorageNumber>
        <Unit>K</Unit>
        <Quantity>Temperature difference</Quantity>
        <Value>-0.180000</Value>
    </DataRecord>

@fredrik-sk fredrik-sk mentioned this pull request Jun 28, 2020
Closed
Apollon77 pushed a commit to Apollon77/libmbus that referenced this pull request Jun 30, 2020
@lategoodbye @Apollon77
Improve BCD decoding (rscada#167)
acd9840 
* Introduce mbus_data_bcd_decode_hex

The convert function mbus_data_bcd_decode (BCD to decimal) suffers from
information loss in case of hexacimal digits. So introduce a new function
mbus_data_bcd_decode_hex (BCD to hexadecimal), which isn't affected and
use this for default XML output. But keep mbus_data_bcd_decode for
normalized output.

(cherry picked from commit 17a7328)
@lategoodbye lategoodbye deleted the improve-bcd-decoding branch July 19, 2020 08:56
lategoodbye added a commit that referenced this pull request Jul 19, 2020
@lategoodbye
Improve BCD decoding (#167)
2a66e6a 
* Introduce mbus_data_bcd_decode_hex

The convert function mbus_data_bcd_decode (BCD to decimal) suffers from
information loss in case of hexacimal digits. So introduce a new function
mbus_data_bcd_decode_hex (BCD to hexadecimal), which isn't affected and
use this for default XML output. But keep mbus_data_bcd_decode for
normalized output.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fredrik-sk fredrik-sk fredrik-sk left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lategoodbye @fredrik-sk