Also generate normalized XML

[lategoodbye]

In order to increase the test coverage also generate the normalized version
of XML output.

[fredrik-sk]

Suggested change

	filename=`basename $hexfile .hex`
	filename=$(basename "$hexfile" .hex)

Shellcheck, a linter for shell scripts using $( ) instead of , and wants you to add quotes where filenames possibly could contain spaces.

[lategoodbye]

Makes sense

[fredrik-sk]

Personal opinion: I would have a dot after filename, and probably keep "xml.new" down here.
Since that is invariant (does not differ over the two cases.

 if [ $mode = "normalized" ]; then
        options="-n"
        suffix="norm"
    else
        options=""
        suffix=""
    fi
  ...
    $mbus_parse_hex $options "$hexfile" > "$directory/$filename.${suffix}xml.new"

[lategoodbye]

I didn't understand the keep xml.new thing. In case the output differ we keep the new file. So we need the suffix, otherwise we will overwrite the new default output.

[fredrik-sk]

What I was trying to say, is that both modes "default" and "normalized" will name the files withe the ending ".xml.new",

I would write it like this:

options = ""
if [ "$mode" = "default" ]; then
    mode=""
elif [ "$mode" = "normalized" ];
    mode=".norm"
    options="-n"
else
    echo "ERROR: Unknown mode $mode."
    return 1
fi
...
$mbus_parse_hex $options "$hexfile" > "$directory/$filename$mode.xml.new"

diff -u "$directory/$filename$mode.xml" "$directory/$filename$mode.xm.new" 2> /dev/null > "$directory/$filename$mode.dif"

[fredrik-sk]

Suggested change

	diff -u "$directory/$filename$suffix" "$directory/$filename$suffix.new" 2> /dev/null > "$directory/$filename.dif"
	diff -u "$directory/$filename$suffix" "$directory/$filename$suffix.new" 2> /dev/null > "$directory/$filename$suffix.dif"

I would like to run all tests at once, and see every failed test at once. Both not nromailized and normalized could fail. So it is better to add the suffix here. So there is no question about which test that failed.

[fredrik-sk]

Found some more things...

[fredrik-sk]

Here we don't seem to look at the return value of the function

[fredrik-sk]

Or did you intend me to update the commit "make build stop on broken tests"?

[lategoodbye]

It was your idea, so you should get the credits for it.

[fredrik-sk]

Suggested change

	return 0
	return $result

[lategoodbye]

I pushed a new version

[fredrik-sk]

Hello

We can definetly handle this as a new issue.
I want to point out what I have seen, and what my opinions are.

I didn't notice this until I rebased my work onto your PR and ran the tests.
There is some file that gives some output

When looking at the tests of this PR, https://github.com/rscada/libmbus/pull/168/checks?check_run_id=816071416

Lines 12-14 of the step "generate test frames" is new.
There is no indication what file is causing this. (The level above probably just ignores it.)

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

Locally, I added this command echo "*** "$hexfile at the loop at the end of generate-xml.sh.

It indicated:

*** test/test-frames/sen_pollutherm.hex

Looking at the diff, indicates that datarecord with id=2 is completly empty in the normalized file.
The contents in the record with id=9, is how the normalized xml should look?

diff -u sen_pollutherm.xml sen_pollutherm.norm.xml | less

     <DataRecord id="1">
         <Function>Instantaneous value</Function>
         <StorageNumber>0</StorageNumber>
-        <Unit>Volume (1e-2  m^3)</Unit>
-        <Value>799892</Value>
+        <Unit>m^3</Unit>
+        <Quantity>Volume</Quantity>
+        <Value>7998.920000</Value>
     </DataRecord>

     <DataRecord id="2">
-        <Function>Instantaneous value</Function>
-        <StorageNumber>0</StorageNumber>
-        <Unit>Unknown (VIF=0x7B)</Unit>
-        <Value>302</Value>
     </DataRecord>
...
     <DataRecord id="9">
         <Function>More records follow</Function>
+        <StorageNumber>0</StorageNumber>
+        <Unit></Unit>
+        <Quantity></Quantity>
         <Value></Value>
     </DataRecord>

I would suggest two actions from this.

• Perhaps avoid checking in sen_pollutherm.norm.xml until this has been solved.
• Implement some way that the error messages could say what file caused the problem. Either by setting the name of the
  file as a global variable, or that this problem results in an exitcode of -1 in mbus_parse_hex.
• Fix this particular VIF if it exists in the standard.

[fredrik-sk]

See comment #168 (comment) .

This is probably where this is printed.

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

[fredrik-sk]

Perhaps skip checking in this file while this issue remains?