GENIVI / CANdb

Library for parsing CAN bus database description formats
Mozilla Public License 2.0
144 stars 46 forks source link

Fixing signal sign handling and functional testing - follow-up #20

Closed jenszo closed 4 years ago

jenszo commented 4 years ago

Disclaimer: Follow-up on PR #19 using a stable branch.

Hi,

I came across somewhat serious issues regarding the handling of a signal's sign bit and the functional tests.

The grammar specified a "sign" but did not distinguish the mandatory "+"/"-" sign of the entire signal from any signs that may be specified along with the factor, offset or range numbers. If there was a "-" in any of these, the "sign" was added to the stack but never used as it was part of a legitimate number. However, when taking the "sign" off the stack, the last added one from any of the numbers from the end of the signal is falsely taken as sign of the entire signal. If I'm not mistaken, orphaned signs might have been carried over to the next signal. Therefore, I've introduced a "sig_sign" in the grammar

I've added my message that has shown the issue to your functional test as a reference. Doing this, I noticed that all your tests succeeded for as long as the name of a signal matched => I've adapted the comparison operator of CANsignal respectively and fixed some of otherwise failing tests.

Strings for the signal sign e.g. "+" and "-" would have been cast to bool and would thus always evaluate to TRUE. To fix this issue and improve overall type safety, I've taken the liberty to introduce backwards-compatible enum symbols for both the signal sign as well as its byte order.

This time, I hope all will build well ;)

Regards, Jens