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

CAN: Fix signness mismatch in `CANMessage` constructors #9111

Merged
merged 2 commits into from Apr 1, 2019

Conversation

Projects
None yet
@embeddedteam103
Copy link
Contributor

commented Dec 16, 2018

Description

Fixed another signness mismatch in CAN driver reported in #7444 and partially fixed in #7848

There is another mismatch (fields should be unsigned) in the data and length members as well (in accord with the definition of CAN_Message in the HAL file: hal\can_helper.h).

The example in the docs is also effected to reflect this change (and not cause a warning in certain builds).

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

@embeddedteam103 embeddedteam103 changed the title Added unsigned everywhere in the dirver Fixed signness mismatch in `CANMessage` constructors Dec 16, 2018

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Dec 16, 2018

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Dec 16, 2018

@embeddedteam103, thank you for your changes.
@ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

There's really no need to stick a U on simple constants like that, but I guess it doesn't hurt. Doesn't fundamentally remove any type conversion, because the constant is still an unsigned int, so is still getting type converted on assignment. As long as the constant value is in range of the assigned-to type, the code is valid.

As for the data pointer change - the data is copied via memcpy anyway, so the input pointer type doesn't really matter. If anything, it should be changed to const void *.

@embeddedteam103

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2018

Well, as for the conversions you are right, it is just a matter of taste.

But for the data I disagree - it definetly should NOT be const void*.

Why? Because if the user holds the data in a type that is not char (say an array of uint32_t) then she must explicitly cast to const unsigned char* (if the API is const void* she does not). Explicit casting here should be mandatory as this is a potential endianess bug - if we require an explicit act from the user, they acknowledge they know what they are doing (and a good compiler will warn them in certain build modes).

Just to be clear: almost every novice in our team has written this exact endianess bug - and because they had to do explicit casts (in other functions in the mbed codebase, and functions in our own codebase) we could spot it in code review and tell them immediately.

If you agree with my rant then the data member should be const char* or const unsigned char* (and NOT const void*).

The data member is unsigned therefore I believe this change is for the best.

A point in favor of unsigned: if you look at these things in the debugger - it might be confusing because the debugger might use different views depending on signness ("I wrote 'X', why does it show 'x' written in memory? Where is the bug???")

@0xc0170

This comment has been minimized.

Copy link
Member

commented Dec 17, 2018

Note: Mbed OS defines char as unsigned. This is taken from older docs

"Whilst most types are signed by default (short, int, long long), char is unsigned by default."

Changes looks fine to me.

Can you fix the commit msg Added unsigned everywhere in the dirver - typo in driver, and use active voice "Add usigned". Also a fix should state why are we adding this - what does this fix addresses and how?

I've fixed the title

@0xc0170 0xc0170 changed the title Fixed signness mismatch in `CANMessage` constructors CAN: Fix signness mismatch in `CANMessage` constructors Dec 17, 2018

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

char here is unsigned, as @0xc0170 says, but it is still a functional change as char and unsigned char are different types regardless, so the pointers are incompatible.

I half-buy the "don't use void * argument", but not totally. It rather depends on the API level. If for this specific protocol, you definitely expect people to be formatting packets manually using unsigned chars, then taking that type makes sense - it's a useful type check, so maybe it's not void * here.

If it was a more abstract API, I'd still want to take void *, as the transfer code should not be imposing any view on how the data is formed. Maybe it is native-endian uint32_ts for a certain protocol.

Now, this is a totally breaking API change - every single caller of this API will need to change their code. All existing users will be passing char arrays and they will all have to be changed to unsigned char, and there's no form that will work with both.

Given that char is defined as unsigned, and other APIs like SPI are still using char *, I'm rather doubtful of the benefit.

The length change, on the other hand, is probably fine.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

A semi-convention in many parts of the code base is that char * means "zero-terminated string" whereas uint8_t * means "arbitrary byte data", and we'd use the latter if not using void *, but the core HAL mbed drivers don't follow that, sadly.

So for me any move away from plain char * is a good one, whether it be to unsigned char *, uint8_t * or void *. (Or std::byte * in C++17?).

Now, I guess there is a backwards-compatible path here which is to add an extra constructor for unsigned char *, deprecating the char * one. Thoughts, @0xc0170?

(Can't have a separate constructor for uint8_t *, as can't guarantee that is a different type from char *).

Add unsigned everywhere in the driver for
compatibility with the CAN HAL

@embeddedteam103 embeddedteam103 force-pushed the embeddedteam103:can_signness_mismatch branch to 0210cef Dec 17, 2018

@embeddedteam103

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2018

Now, this is a totally breaking API change - every single caller of this API will need to change their code. All existing users will be passing char arrays and they will all have to be changed to unsigned char, and there's no form that will work with both.

I'm not sure - people using the HAL directly will use unsigned char. People building the data from memory will probably use uint8_t (or std::byte in an ideal world - we have a typedef for byte which is unsigned char).
I think the fact that right now the APIs diverge causes most calls to follow a cast (explict or implicit - depends on warning level), and therefore this change will reduce the number of casts. But this is just speculation :)

I think having both overloads with one deprecated is acceptable (unless it will cause wierd conversion issues).

A counter point would be that codebases with a byte typedef or something similar couldn't gurantee they use the right overload without an explicit cast to unsigned char (which will increase the number of casts in the call).

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

I'm not sure

Well, it's not breaking for people using other APIs. But it would break every existing user of this particular API - "break" meaning requiring an application change to get valid C++ code again, even if it is trivial.

(Although despite being explicitly illegal C++, at least some compilers will let the pointer type mismatch slip with only a warning as their required diagnostic. Not sure about the state of all 3 toolchains).

I think the fact that right now the APIs diverge causes most calls to follow a cast

Surely most code uses only the C++ API or only the HAL API? Would expect there be to much that uses both. Or is the C++ API incomplete, so forces use of the HAL API too or something? Never looked at this CAN stuff before.

Anyway, whatever they're casting to or using directly now will become wrong for this call, unless we provide the overload. I don't think there's any issue doing that. They're two clearly distinct types, as long as we don't go near uint8_t. And, actually, if you didn't deprecate either, then that would basically make uint8_t legal, whether it's char or unsigned char...

@embeddedteam103

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2018

So to proceed, I should add both char and unsigned char overloads, with the signed deprecated?
What version to put in the deprecation warning? 5.11?

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2018

Just to be clear - you do realise there is no signed version? char is unsigned (by default for ARMs, and by definition in Mbed OS).

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2018

I think I want some feedback from @ARMmbed/mbed-os-hal here.

My current view is that if we want CAN data to always be viewed as unsigned bytes (do we?), then it may make sense to lock that type by using an unsigned 8-bit type, rather than void *.

But I don't see a particular reason to coerce people towards unsigned char specifically. Could people not be sending ASCII text data, which will be naturally stored in a char array?

Forcing people to put in casts for that case causes just as many problems, so I'd be inclined to have overloads for both char and unsigned char, and deprecate neither.

I'd also want people to be able to pass uint8_t * pointers - do all toolchains define this as unsigned char? If any have it as char, I'd want to not lose or deprecate the char to keep that okay.

@embeddedteam103

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2018

Just to be clear - you do realise there is no signed version? char is unsigned (by default for ARMs, and by definition in Mbed OS).

Yes of course. But at the type system level these are different (and for overload resolution, which is the case here).

Forcing people to put in casts for that case causes just as many problems, so I'd be inclined to have overloads for both char and unsigned char, and deprecate neither.

I think this is not intuitive at an API level - I think the sole overload should be the type given in the underlying HAL object and anyone using a different type should explicitly cast (even when casting to void* but this is just a convention in this case).
Also this kind of dual API is not given in any place I know of in mbed.

My current view is that if we want CAN data to always be viewed as unsigned bytes (do we?), then it may make sense to lock that type by using an unsigned 8-bit type, rather than void *.

Do you mean changing the member in the HAL struct?

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2018

the sole overload should be the type given in the underlying HAL object

I really don't think that follows. The HAL layer is a comms layer that treats it as opaque data. It's irrelevant how it chooses to handle it.

The C++ layer is free to have a bunch of overloads that do the right thing for a bunch of data formats. It could have a constructor to make a CAN message that takes a uint32_t, if there's a conventional way to pack a 32-bit integer in a CAN message.

You could also have a constructor that takes a char * and no length, uses strlen on it, and packs it into a message (faulting if strlen > 8).

I've no idea what the possible relevant data formats for CAN are, but there's no requirement for the only CAN_Message constructor to be a "raw data" one. The C++ APIs can be richer than the HAL ones.

lock that type by using an unsigned 8-bit type

No, I mean the type for the constructor.

@embeddedteam103

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2018

I've no idea what the possible relevant data formats for CAN are, but there's no requirement for the only CAN_Message constructor to be a "raw data" one. The C++ APIs can be richer than the HAL ones.

Fair enough. But the one constructor available now is supposed to do the most basic behavior - "raw data only".

If I understand you properly - the reason for no deprecation of one of the constructor is that in a possible extension (where having multiple constructors with different behaviors) one of these constructors (not a "raw data" constructor) will do something else, but have the same signature?

And we are waiting for CAN HAL maintainers to weigh in on whether this extension is a good idea - to decide if we go for multiple constructors with different behaviors OR it is not needed and only a "raw data" constructor is needed (and then we go the deprecation path)?

I am by no means an expert on packaging CAN data, so I won't have much to say in this regard.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2018

the one constructor available now is supposed to do the most basic behavior - "raw data only".

Yes, and that seems like it should accept unsigned char or uint8_t, so a new overload seems reasonable. (And in C++17 it should also have a byte overload). But I feel that same argument would apply to any other HAL raw block data APIs that currently take char * rather than void * (and if they're not to be changed to void *), so it's a general HAL issue as to whether they agree with that principle.

char *, len is a perfectly good prototype, and already has a reasonable meaning - it puts text data in the packet without changing its encoding, 1 character per byte. There's nothing wrong with doing that, conceptually, for this interface type, so I don't see a reason to deprecate it.

I'm awaiting more feedback because I suspect a lot of people might not buy the basic typing argument and would say "just make it void *".

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-hal Jan 4, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

I am not certain how to proceed here. @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-core Can you review?

@deepikabhavnani

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2019

Sorry for delayed response. I will totally buy the argument for not having void *.
Overload with char and unsigned char with none deprecated seems good for now.

@bulislaw

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

@deepikabhavnani @kegilbert can you actually approve the changes if you are ok with them? HAL review is not necessary as this PR only touches drivers.

@deepikabhavnani

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2019

Overload with char and unsigned char with none deprecated seems good for now.

This is still needed, instead of replacing const char * to const unsigned char * add a overload option.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

@embeddedteam103 Please see @deepikabhavnani's comment.

@c1728p9
Copy link
Contributor

left a comment

Marking this as Request changes until the overload mentioned earlier is added.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

@embeddedteam103 Are there any available updates on this PR?

CoolUsername
@embeddedteam103

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

Sorry for delayed response. For some reason my notification for this PR was muted.
Added the overload

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

@mbed-os-core @c1728p9 When y'all get a chance.

@cmonr cmonr requested a review from ARMmbed/mbed-os-hal Mar 27, 2019

@cmonr cmonr added needs: CI and removed needs: review labels Mar 29, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

CI started

@cmonr

cmonr approved these changes Mar 29, 2019

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 29, 2019

Test run: FAILED

Summary: 1 of 13 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test
@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2019

seems to be a K66 CI issue. restarting CI to check

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 31, 2019

Test run: FAILED

Summary: 1 of 13 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test
@alekla01

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

restarted jenkins-ci/mbed-os-ci_greentea-test

@mbed-ci

This comment has been minimized.

Copy link

commented Apr 1, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 3
Build artifacts

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Apr 1, 2019

@cmonr cmonr merged commit d2c9d9d into ARMmbed:master Apr 1, 2019

28 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARMC5 Success
Details
jenkins-ci/mbed2-build-ARMC6 Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 10214 cycles (+1115 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Apr 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.