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

Fixed Cryptic Error Codes in mpStartJob #106

Merged
merged 7 commits into from
Aug 3, 2023

Conversation

aigallegos
Copy link
Contributor

fixes #102. I wrote a new function that kept the error string format as an f-string for mpStartJob errors in MotionControl.c by just adding a new function called Ros_ErrorHandling_Only_ErrNo_ToString in ErrorHandling.c that did not use strncpy and therefore did not need a buffer to get passed in, similar to Ros_ErrorHandling_MotionNotReadyCode_ToString it just returns a char* instead of having a destination for the chars and can be directly passed into the error f-string.

src/ErrorHandling.c Outdated Show resolved Hide resolved
src/MotionControl.c Outdated Show resolved Hide resolved
@gavanderhoorn
Copy link
Collaborator

I wrote a new function that kept the error string format as an f-string for mpStartJob errors in MotionControl.c by just adding a new function called Ros_ErrorHandling_Only_ErrNo_ToString in ErrorHandling.c that did not use strncpy and therefore did not need a buffer to get passed in

thing is, that buffer & copy are needed, as functions can't return pointers to temporaries (ie: variables on the stack).

similar to Ros_ErrorHandling_MotionNotReadyCode_ToString it just returns a char* instead of having a destination for the chars and can be directly passed into the error f-string.

you could consider defining all error 'strings' as

static char something[] = "...";

which would allow just return-ing something directly.

Ros_ErrorHandling_MotionNotReadyCode_ToString(..) used to do this, but that's invisible now, as the storage for the 'strings' returned by it is the responsibility of the code generated by the ROS 2 message generators now.

Literals are not local objects.
According to the C standard (99 standard 6.4.5/5 "String Literals - Semantics"), they are automatically stored in an "anonymous array of static storage".

Returning const pointers to literals should therefore be OK and we can avoid the temporary char[] and strncpy(..) completely.
ErrNo_ToString(..) implements the same functionality.
@gavanderhoorn
Copy link
Collaborator

Turns out, string literals are treated differently from other locals. According to the C99 standard, they are automatically added to an anonymous array with static storage, so returning const char* const to elements should be OK. TIL.

(note: only literals get this treatment. Returning pointers to anything else on the stack would still be problematic)

@aigallegos: your initial implementation was already OK, so apologies for the noise.

I did remove the function you added though, as it was basically a duplicate of Ros_ErrorHandling_ErrNo_ToString(..) and updated your PR to now just call the updated Ros_ErrorHandling_ErrNo_ToString(..).

@gavanderhoorn
Copy link
Collaborator

Note: we should consider squash-merging this if possible. All my commits are essentially fix-ups.

@gavanderhoorn gavanderhoorn dismissed their stale review August 3, 2023 10:11

I've implemented my own requested changes, need a fresh pair-of-eyes to review

Copy link
Collaborator

@ted-miller ted-miller left a comment

Choose a reason for hiding this comment

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

Looks good.

THU 2023-08-03 15:03:27.596 Ros_MotionControl_StartMotionMode: enter
THU 2023-08-03 15:03:29.876 Can't start 'INIT_ROS' because: 'The home position is not registered' (0x3040)

Thank you both for the PR!

@ted-miller ted-miller merged commit 5a2c6ee into Yaskawa-Global:main Aug 3, 2023
5 checks passed
@gavanderhoorn
Copy link
Collaborator

Thanks @aigallegos 👍

@aigallegos
Copy link
Contributor Author

Thanks @aigallegos 👍

Thank YOU! I learned so much from this rabbit hole we went down and thanks for putting up with my newbie negligence that just so happened to be correct haha. Thanks for the help and happy coding!

@gavanderhoorn
Copy link
Collaborator

If you see any other issues you believe you could help with, please comment on them and we can discuss potential approaches.

@gavanderhoorn gavanderhoorn added this to the 0.1.1 milestone Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cryptic error codes from mpStartJob
3 participants