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

iox-#1295 Rework icediscovery example #1219

Merged

Conversation

mossmaurice
Copy link
Contributor

@mossmaurice mossmaurice commented Mar 3, 2022

Pre-Review Checklist for the PR Author

  1. Code follows the coding style of CONTRIBUTING.md
  2. Tests follow the best practice for testing
  3. Changelog updated in the unreleased section including API breaking changes
  4. Branch follows the naming format (iox-#123-this-is-a-branch)
  5. Commits messages are according to this guideline
    • Commit messages have the issue ID (iox-#123 commit text)
    • Commit messages are signed (git commit -s)
    • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
  6. Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  7. Relevant issues are linked
  8. Add sensible notes for the reviewer
  9. All checks have passed (except task-list-completed)
  10. Assign PR to reviewer

Notes for Reviewer

  • Fix typos
  • Fix line length
  • Rename event id of 'Radar' service to 'Objects'

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Code according to our coding style and naming conventions
  • Unit tests have been written for new behavior
    • Each unit test case has a unique UUID
  • Public API changes are documented via doxygen
  • Copyright owner are updated in the changed files
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

@mossmaurice mossmaurice added the documentation Improvements or additions to documentation label Mar 3, 2022
@mossmaurice mossmaurice self-assigned this Mar 3, 2022
@codecov
Copy link

codecov bot commented Mar 3, 2022

Codecov Report

Merging #1219 (677bb5a) into master (262e631) will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1219      +/-   ##
==========================================
+ Coverage   78.98%   79.02%   +0.04%     
==========================================
  Files         374      374              
  Lines       14687    14687              
  Branches     2051     2051              
==========================================
+ Hits        11601    11607       +6     
+ Misses       2412     2406       -6     
  Partials      674      674              
Flag Coverage Δ
unittests 78.25% <ø> (+0.04%) ⬆️
unittests_timing 15.45% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
iceoryx_hoofs/source/posix_wrapper/timer.cpp 65.10% <0.00%> (-0.43%) ⬇️
iceoryx_posh/source/roudi/port_manager.cpp 85.36% <0.00%> (+1.12%) ⬆️
iceoryx_hoofs/source/concurrent/loffli.cpp 91.42% <0.00%> (+2.85%) ⬆️

@mossmaurice mossmaurice force-pushed the iox-#743-rework-icediscovery-example branch from 29483c8 to 0d98df8 Compare March 4, 2022 09:26
Copy link
Contributor

@MatthiasKillat MatthiasKillat left a comment

Choose a reason for hiding this comment

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

If we change the outputs (why?), we need to update the integration tests ...

iceoryx_examples/icediscovery/README.md Show resolved Hide resolved
iceoryx_examples/icediscovery/README.md Show resolved Hide resolved
MatthiasKillat
MatthiasKillat previously approved these changes Mar 4, 2022
@mossmaurice mossmaurice changed the base branch from master to release_2.0 March 7, 2022 20:55
@mossmaurice mossmaurice dismissed MatthiasKillat’s stale review March 7, 2022 20:55

The base branch was changed.

@elBoberido
Copy link
Member

Please change the target branch to master

@mossmaurice
Copy link
Contributor Author

Please change the target branch to master

@elBoberido I don't think that's a good idea. I propse to bring the certain small bug/doc fixes onto release_2.0 first, and then backport them to master.

@mossmaurice mossmaurice changed the base branch from release_2.0 to master March 14, 2022 13:20
@mossmaurice
Copy link
Contributor Author

As discussed, we won't do any major changes on the release_2.0. Hence, I changed the target branch to master.

Copy link
Contributor

@elfenpiff elfenpiff left a comment

Choose a reason for hiding this comment

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

When the `find

@mossmaurice mossmaurice force-pushed the iox-#743-rework-icediscovery-example branch from 355e746 to 2a87e5b Compare March 17, 2022 13:03
@mossmaurice
Copy link
Contributor Author

mossmaurice commented Mar 17, 2022

I'll re-work this as part of #1295 and close this PR once there is a new one. Please do not review.

@elfenpiff
Copy link
Contributor

I'll re-work this as part of #1295 and close this PR once there is a new one. Please do not review.

@mossmaurice

Wouldn't it be much better for the reviewers when you would use this PR to make the changes. Otherwise comments are lost and we may loose some hints and suggestions from this PR since they are not transferred.

@mossmaurice mossmaurice force-pushed the iox-#743-rework-icediscovery-example branch from 2a87e5b to 6fe964b Compare March 29, 2022 09:55
@mossmaurice
Copy link
Contributor Author

@elfenpiff

Wouldn't it be much better for the reviewers when you would use this PR to make the changes. Otherwise comments are lost and we may loose some hints and suggestions from this PR since they are not transferred.

I'm torn between here. Of course you are right, closing a PR and opening a new one is unhandy because developers have to keep track of findings in both PRs. On the other hand my intention for closing this PR here was to ensure proper traceability. The branch name still contains a issue #743 which was closed after the Blueberry release and should not be used for v3.0. I adapted all commit messages to the new release validation issue #1295 so it's fine IMHO traceability-wise.

Please go ahead with the review if you agree, otherwise I can also close and re-open with the correct branch name.

@mossmaurice mossmaurice force-pushed the iox-#743-rework-icediscovery-example branch 3 times, most recently from 807fb97 to 4d018c8 Compare March 30, 2022 08:58
@mossmaurice mossmaurice changed the title iox-#743 Rework icediscovery example iox-#1295 Rework icediscovery example Mar 30, 2022
@mossmaurice mossmaurice force-pushed the iox-#743-rework-icediscovery-example branch from 4d018c8 to cd6e531 Compare March 30, 2022 10:32
…entId to 'Objects' for Radar service

Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
…ing of topic

Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
…ordingly

Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
@mossmaurice mossmaurice force-pushed the iox-#743-rework-icediscovery-example branch from cd6e531 to 117086e Compare March 30, 2022 20:05
Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
@@ -42,10 +46,10 @@ We can now call three different find service functions. Let's start with
<!--[geoffrey][iceoryx_examples/icediscovery_in_c/iox_c_find_service.c][find service and apply callable]-->
```c
iox_service_discovery_find_service_apply_callable(
serviceDiscovery, "Radar", "FrontLeft", "Image", printSearchResult, MessagingPattern_PUB_SUB);
serviceDiscovery, "Radar", "FrontLeft", "Objects", printSearchResult, MessagingPattern_PUB_SUB);
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we had this discussion but please note that here actually are radar images in the scientific literature. They are just tensors of F^nxmxk. With F some field, mainly real or complex numbers.

E.g. https://en.wikipedia.org/wiki/Synthetic-aperture_radar.

It was perfectly correct, and this change has no value. For the example it does not matter so, so we keep it now.

Copy link
Contributor Author

@mossmaurice mossmaurice Apr 11, 2022

Choose a reason for hiding this comment

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

@MatthiasKillat I know what you mean. IMHO what we demonstrate here is an example right before the perception module rather in the middle of the feedback loop and not in the front where the low-level computation on the raw radar data happens. Typically radar sensors will only provide object lists over the network to be consumed by a central ADAS.

@mossmaurice mossmaurice dismissed elfenpiff’s stale review April 11, 2022 12:39

Findings addressed

@mossmaurice mossmaurice merged commit 44f1dbc into eclipse-iceoryx:master Apr 11, 2022
@mossmaurice mossmaurice deleted the iox-#743-rework-icediscovery-example branch April 11, 2022 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Release validation for v3.0
5 participants