-
Notifications
You must be signed in to change notification settings - Fork 50
Fix compilation errors #92
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
Conversation
| case fpsdk::common::parser::nmea::NmeaSignalId::GLO_L1OF: return msg.consts.SIGNAL_ID_GLO_L1OF; | ||
| case fpsdk::common::parser::nmea::NmeaSignalId::GLO_L2OF: return msg.consts.SIGNAL_ID_GLO_L2OF; | ||
| case fpsdk::common::parser::nmea::NmeaSignalId::NAVIC_L5A: return msg.consts.SIGNAL_ID_NAVIC_L5A; | ||
| default: return msg.consts.SIGNAL_ID_UNSPECIFIED; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang complains about unhandled cases otherwise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add and handle all enum values here and not use a default case. I'll fix that.
| if (pub->get_subscription_count() > 0) { | ||
| fpmsgs::NmeaGga msg; | ||
| msg.talker = NmeaTalkerIdToMsg(msg, payload.talker); | ||
| msg.talker = NmeaTalkerIdToMsg(msg, payload.talker_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you want to align conventions and remove the underscore from talker_, being this a public member
| msg.latitude = (payload.ll.latlon_valid ? payload.ll.lat : NAN); | ||
| msg.longitude = (payload.ll.latlon_valid ? payload.ll.lon : NAN); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
llh does not exist on this struct
| // Output jump warning | ||
| if (params_.cov_warning_ && odometry_data.valid && jump_detector_.Check(odometry_data)) { | ||
| RCLCPP_WARN(logger_, jump_detector_.warning_.c_str()); | ||
| RCLCPP_WARN(logger_, "%s", jump_detector_.warning_.c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second argument should be a format string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's better. Thanks.
|
Hi. The fixpostion_driver is tested and linked to a specific version (commit) of the Fixposition SDK. If you use the correct commit in the SDK, most of the compilation problems you see won't appear. Basically all but the "clang stuff" should not be a problem (so says our CI [1], see also [2]). For this purpose the fixposition_driver repo comes with the SDK as a git submodule, which links it to the correct version (commit). The non-clang compilation problems you see are very recent changes/additions/fixes to the SDK. We'll update the fixposition_driver accordingly. Until then, please use this Fixposition SDK commit. I'll have a look at that in the next days/week(s). [1] https://github.com/fixposition/fixposition_driver/actions/runs/18140587873 |
|
(OK, I see now that clang related problems are only in the SDK.) |
Attempt 1 - Latest driver tag and SDK pointed to from the driver commit
Attempt 2: Building fpsdk-common on the commit suggested in the previous commentfpsdk-common: 53fa5b5 Attempt 3: Using the latest fpsdk-commonfpsdk-common: 408dc893a Attempt 4: Latest fpsdk-common and latest fixposition-driver (solving manually clang warnings)fpsdk-common: 408dc893a So, currently there is no combination that compiles for us. I think the problem is the following:
|
|
I haven't tried all your combinations, but for me using driver tag 8.0.2 and SDK at 4822a88 works fine (Using GCC, not clang...). There's no "ll" vs "llh" issue. I think you have another commit checked out for the SDK there This is currently the latest supported and tested driver release (as far as I know..). Clearly, it doesn't support clang. So you'd have to use GCC if you wanted to use that. |
|
Can you check if this branch works for you (using clang): https://github.com/fixposition/fixposition_driver/tree/feature/bump-sdk What's the command to "colcon build" using clang? |
|
I have merged #93, i.e. main should now work with (today's) main of the fixposition-sdk repo. This is for GCC and only the changes that were required to update for the changes in the SDK (talker_ vs talker, ll vs llh). The fixes for clang (that missing "%s") as well as some CI for clang is coming here: #94 |
|
OK, I merged this: #94 Note that none of this is thoroughly tested. The latest fully tested and known-working version still is tag 8.0.4 (with corresponding commit of the fixposition-sdk). I'm closing this PR as all the changes are now in main by means of #93 and #94. |

This solves various compilation errors. Probably as I am building with clang, I am seeing themwhile they probably went unnoticed when using gcc.
This is coupled with fixposition/fixposition-sdk#156
I am also a bit confused. How was this compiling before? I have just noticed that main is not working with the latest of fixposition_sdk. This has been compiling with the latest
fixposition_sdk(patched with the referenced PR).I would appreciate if you could make a new release incorporating these changes, so that also clang-based build can work. Thanks for your support!