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

Integer attributes should support 64 bit signed integers to be compliant with CANdb++ #91

Open
sToman967 opened this issue Sep 11, 2024 · 10 comments
Labels
enhancement New feature or request

Comments

@sToman967
Copy link

In PropertiesDefinitionLineParser, when checking match.Groups[IsIntegerValueGroup].Value.Equals("INT") and
match.Groups[IsIntegerValueGroup].Value.Equals("HEX") ,
the strings are parsed as int:
Minimum = int.Parse(match.Groups[MinIntGroup].Value, CultureInfo.InvariantCulture), Maximum = int.Parse(match.Groups[MaxIntGroup].Value, CultureInfo.InvariantCulture), ,
but some values in the dbc I am trying to parse are too large to be stored in int32. They need to be parsed into long.
If there is any way to address this, would be much appreciated. Thank you!

@Adhara3
Copy link
Collaborator

Adhara3 commented Sep 11, 2024

Hi,

the specs say so, actually

attribute_value_type = 'INT' signed_integer signed_integer |
   'HEX' signed_integer signed_integer |
   'FLOAT' double double |
   'STRING' |
   'ENUM' [char_string {',' char_string}]

we may switch to long but it would be an undocumented feature.

What about other parsers? Is CANdb++ opening it right ?

Thanks
A

@Adhara3 Adhara3 added the enhancement New feature or request label Sep 11, 2024
@Adhara3
Copy link
Collaborator

Adhara3 commented Sep 11, 2024

In CANdb++ Editor (v3.1.25 SP3) this

BA_DEF_ SG_ "PName" INT 0 12591421498;
BA_DEF_DEF_ "PName" 0;

is parsed and displayed like this
image

Cheers
A

@sToman967
Copy link
Author

Hi,

thank you for your answer.
Yes, candb++ does parse these values correctly. After looking over it again, I realized that only the hexadecimal values are presenting an issue here, for example, trying to parse this into an int32:
BA_DEF_ SG_ "PrivSig_NAValue" HEX 0 4294967295; , being treated as a decimal value as far as I can understand, causes an overflow exception. I have tried storing it into long and it works.

@Adhara3
Copy link
Collaborator

Adhara3 commented Sep 12, 2024

Hi,

Yes, candb++ does parse these values correctly

This is not what I'm experiencing with latest CANdb++ Editor. I tried both

BA_DEF_ SG_  "PrivSig_NAValue_HEX" HEX 0 4294967295;
BA_DEF_ SG_  "PrivSig_NAValue_INT" INT 0 4294967295;

The INT is not parsed correctly (it's not a parsing issue, more a numeric casting one), while the HEX value is fine.
image

After all 4294967295 is nothing but uint.MaxValue so the behavior is consistent with HEX being unsigned.
So, if you agree, we can change the code so that HEX types accept UINT (so no negative values will be accepted anymore), which is reasonable being the HEX format unsigned by design.

Cheers
A

@Uight
Copy link
Contributor

Uight commented Sep 12, 2024

i wouldnt change the value to u32 since spec says signed integer for both HEX and Int and as is understand the prevois screenshot candb++ parses it without error but still displays uint32.max as int32 min so thats what i would try to do.
For Hex try parse to int if not working tryparse to Uint if thats not working fail. and for int tryparse int then tryparse to unint if unint parse work do an unsafe convert to int32. or keep both the same and tryparse uint32 aswell but than do a unsafe convert to int. (Hex would stay the same andyway)

for api 2.0 maybe change HEX to be HEX so save as string? the problem would then be solved completly.

@Adhara3 Adhara3 changed the title Some values are too large for int32 when parsing properties definition lines HEX attributes should support 32 bit unsigned values instead of signed ones Sep 12, 2024
@Adhara3
Copy link
Collaborator

Adhara3 commented Sep 12, 2024

Ok, will parse both as long then.

A

@Uight
Copy link
Contributor

Uight commented Sep 12, 2024

If parsed as long you should then probably check that they are not above uint32.max? or just do an unsafe cast back to int32? but then you would get sometthing totally wrong sometimes

@Adhara3
Copy link
Collaborator

Adhara3 commented Sep 12, 2024

If parsed as long you should then probably check that they are not above uint32.max? or just do an unsafe cast back to int32? but then you would get sometthing totally wrong sometimes

But if I do check, then what happens in user example with max value 4294967295 well above the limit? Should we fail? But failure is what is happening now and we are saying that we do not like that.... I'm a bit confused.

If we want to mimic CANdb++ then we should relax the signed integer (32 bit) constraint admitting that nowhere in the spec 32 bit is cited and after all long is a signed integer type, just 64 bits.

Note

Please also note that CANdb++ is showing that stuff in the UI, but if I save the dbc back to disk, the original values are stored, which means that internally CANdb++ is keeping the original values and that the cast/crop/whatever is only performed in the UI (which is nevertheless bad from a user perspective since you see wrong values).

So if in a HEX I set -25 as minimum, CANdb++ shows it as 0xFFFFFFE7 which of course has no sign concept whatsoever, but we obtain the same thing if we do

var value = -25L;
Console.WriteLine(value.ToString("X"));

A

@Adhara3 Adhara3 changed the title HEX attributes should support 32 bit unsigned values instead of signed ones Integer attributes should support 64 bit signed integers to be compliant with CANdb++ Sep 12, 2024
@Uight
Copy link
Contributor

Uight commented Sep 12, 2024

4294967295 is exactly uint32.max. so i would fail if its over that.

Main reson for my convern is that if you safe it as long now that is kind of breaking as all functions users wrote using custom attributes probably expect a int and long to int is no implicit cast.

But if you allow to read unit32.max and just safe it as normal int youd not get a (breaking) change and the display would be the same as in your screenshot. But your right if you safe it back or anything it would be the wrong value.
could you check how vector handles values even bigger than u32.max? (particular if it stores or writes it back to file it when edited => with your example from above with: 12.591.421.498 which is bigger than Uint32.max whats the behavior?)

from what i get the issue is more linked to Hex values being the uintw.32 max value and as Hex itself has not concept of being signed or unsiged we could allow a max of uint32.max for now and safe it as int32. (bit and therefor hex representaion would be the same still and it wouldnt case an error)

@Adhara3
Copy link
Collaborator

Adhara3 commented Sep 12, 2024

4294967295 is exactly uint32.max. so i would fail if its over that

Ok but then we have broken the signed integer rule, this value is well outside 32 bit signed range

you check how vector handles values even bigger than u32.max?

They handle them. I tried 42949672950 (note last 0 just to make it big). CANdb++ UI shows  0xFFFFFFFF so no more than 32 bits but when saved back the big number is written

from what i get the issue is more linked to Hex values being the uintw.32 max value and as Hex itself has not concept of being signed or unsiged we could allow a max of uint32.max for now and safe it as int32. (bit and therefor hex representaion would be the same still and it wouldnt case an error)

Ok but what about the minimum? 0? int32.MinValue? CANdb++ supports -25 for HEX
I agree that the change to long would change API so can be postponed to v2.0 nevertheless other solutions seem asymmetric to me.

A

Repository owner deleted a comment from sToman967 Sep 20, 2024
Repository owner deleted a comment from sToman967 Sep 20, 2024
Repository owner deleted a comment from Whitehouse112 Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants