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.

Sign up for GitHub

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

Jump to bottom

feat(ars548): initial driver #123

Merged
merged 55 commits into from
Mar 28, 2024
Merged

feat(ars548): initial driver #123

merged 55 commits into from
Mar 28, 2024

Conversation

knzo25
Copy link
Collaborator

@knzo25 knzo25 commented Mar 5, 2024 •
edited by mojomex
Loading

PR Type

  • New Feature

Related Links

Description

Adds the initial hardware interface, decoder and ros2 wrapper for the continental ARS548 radar

Review Procedure

Remarks

Pre-Review Checklist for the PR Author

PR Author should check the checkboxes below when creating the PR.

  • Assign PR to reviewer

Checklist for the PR Reviewer

Reviewers should check the checkboxes below before approval.

  • Commits are properly organized and messages are according to the guideline
  • (Optional) Unit tests have been written for new behavior
  • PR title describes the changes

Post-Review Checklist for the PR Author

PR Author should check the checkboxes below before merging.

  • All open points are addressed and tracked via issues or tickets

CI Checks

  • Build and test for PR: Required to pass before the merge.

knzo25 and others added 28 commits December 13, 2023 15:42
@knzo25
feat: initial implementation of the continentla ars548 driver
ca66948 
The initial implementation consists of a working hw_interface, decoder, and ros wrappers.
The full ros wrappers to be integrated with other coebases have not been implemented.
Since this is the first non-lidar sensor in the codebase, some structural changes were tentatively proposed.
Some refactoring is still needed

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
chore: refactor before sending PR
7e3f2f7 
Changed the generic naming to ARS548 since it looked like many things can not be generalized
Added documentation to the new methods
Addressed remaining TODOs

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
fix: fixed errors with naming and symbols after the refactoring
ad02d9a 
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
chore: fixed spell misses
3d071a2 
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
chore: more spell fixes
c08b660 
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
fix: updated the build_repos to point to the custom transport drivers…
b197664 
… for this PR

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
chore: removed duplicated message file
b7f36e3 
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25 @mojomex
Update nebula_ros/src/continental/continental_ars548_hw_interface_ros…
b672eb5 
…_wrapper.cpp

Co-authored-by: Max Schmeller <6088931+mojomex@users.noreply.github.com>
@knzo25
chore: addressed comments from the review related con consitency and …
ee0802c 
…unused code

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
Merge branch 'feat/continental_drivers' of github.com:knzo25/nebula i…
182c571 
…nto feat/continental_drivers
@knzo25
chore: removed default parameters in line with the awf guidelines
31703e5 
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
chore: replaced the confusing position orientation name by simply oti…
d383931 
…entation

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
chore: removed unused using
0096618 
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
feat: implemented boost endian approach
2ee484f 
Also fixed some typos
Both implementations provide the same results

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
chore: removed the old implementation
475a057 
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
Merge branch 'main' into feat/continental_drivers
6d3fa3b
@knzo25
chore: mixed misspells
4ce4874 
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
Merge branch 'feat/continental_drivers' of github.com:knzo25/nebula i…
4475d2b 
…nto feat/continental_drivers
@knzo25
fix: fixed short circuit in conditional statement, missing lock, and …
2338af7 
…typo

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
feat: added use_receive_time options, enriched the PointCloud2 messag…
8a0f944 
…es for evaluation, and implemented some ROS logic to set the radar configuration and dynamic input

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
Merge branch 'main' into feat/continental_drivers
b84f6a2
@knzo25
feat: added the missing steerin angle input
2506fb7 
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
feat: missing features for proper vehicle integration. fixed fixes an…
871b78a 
…d added visualization markers

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
fix: had implemented the reference point correction for all relevant …
cfd2b7c 
…interfaces but the radar track msgs

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
feat: implemented changes from another branch (for the ars548). diano…
098f400 
…stics are now parsed in the decoder, fixed radar configuration, and implemented a temporary multi radar hw interface

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
Merge branch 'main' into feat/continental_drivers
858c011
@knzo25
chore: spell fix
2801c8b 
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
Merge branch 'feat/continental_drivers' of github.com:knzo25/nebula i…
95ef8a2 
…nto feat/continental_drivers
@knzo25 knzo25 requested review from mojomex and drwnz March 5, 2024 05:27
mojomex
mojomex reviewed Mar 15, 2024
View reviewed changes
Copy link
Collaborator

@mojomex mojomex left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Except for the few small things noted above, this looks really nice!
I'll test with a PCAP or the real sensor asap for some final checks!

Also thanks for doing proper error detection/handling for all the config and sensor comms 😄

@knzo25
chore: spell fix
851dd86 
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
chore: fixed another spelling
aeaad50 
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
chore: yet another spell fix
764e584 
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@mojomex
Copy link
Collaborator

mojomex commented Mar 15, 2024 •
edited
Loading

Confirmed working with real hardware (used this launch file with the sensor's IP changed accordingly) while Autoware was running (for odometry etc.).

Tested:
🟢 All topics output data, no obvious errors (NaN, inf, etc.) in output
🟢 All Rviz topics are displayed correctly
🟢 People / retro-reflectors moving in front of the sensor are shown
🟢 Output fields in the pointcloud have no obvious errors
🟢 No errors logged in console
🟢 Unit tests pass

Service calls etc. have not yet been tested because of the open conversation above.

@knzo25
chore: deleted SRR files that leaked into the ARS branch
1007571 
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
feat: added an option to set all the parameters directly via services
423112d 
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
Copy link
Collaborator Author

knzo25 commented Mar 18, 2024

@mojomex
Applied the requested changes and proposed a new way to set the parameters 🙏

@knzo25
chore: updated cspell dict
fb1b08b 
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@mojomex
Copy link
Collaborator

mojomex commented Mar 18, 2024 •
edited
Loading

🟢 Code review and functionality review complete, LGTM

🟢 Service calls are looking good, too!
🟡 As there is still the bug related to mounting angles (the sensor's fault as far as we know), I'm waiting for @knzo25's feedback on whether to merge (with an added disclaimer about angle limits) or to wait.

@mojomex mojomex self-requested a review March 18, 2024 07:01
@knzo25 knzo25 requested a review from mojomex March 19, 2024 11:09
@knzo25 knzo25 mentioned this pull request Mar 19, 2024
Merged
@mojomex
Copy link
Collaborator

mojomex commented Mar 21, 2024

I'm waiting for @knzo25's feedback on whether to merge (with an added disclaimer about angle limits) or to wait.

@knzo25
If I recall correctly, issues only occur in the object output if the mounting angle of the radar is > 20deg off the longitudinal axis, correct?
Would you mind adding a line to the readme warning users of that? Everything else already looks good 👍

@knzo25
Copy link
Collaborator Author

knzo25 commented Mar 26, 2024

@mojomex
I got the thumbs up from David to merge this :)
Regarding the erratic behavior, since nothing is confirmed, I prefer to fix and document it once we hear back from Continental

mojomex
mojomex approved these changes Mar 28, 2024
View reviewed changes
Copy link
Collaborator

@mojomex mojomex left a comment

Choose a reason for hiding this comment

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

LGTM!

@mojomex
Copy link
Collaborator

mojomex commented Mar 28, 2024

@amc-nu It looks like all requested changes are addressed, is it okay to go ahead and merge?

@amc-nu
Copy link
Contributor

amc-nu commented Mar 28, 2024

Yes, please go ahead!
Thank you @knzo25

@knzo25 knzo25 merged commit cb3c56a into tier4:main Mar 28, 2024
6 checks passed
@knzo25 knzo25 deleted the feat/continental_drivers branch March 28, 2024 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amc-nu amc-nu amc-nu approved these changes

@mojomex mojomex mojomex approved these changes

@drwnz drwnz Awaiting requested review from drwnz

Assignees

@knzo25 knzo25

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@knzo25 @codecov-commenter @amc-nu @mojomex