Skip to content

echo_pulse_width added in osi_featuredata.proto for message LidarDetection #511

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

Merged
merged 5 commits into from
Oct 14, 2021

Conversation

PhRosenberger
Copy link
Contributor

@PhRosenberger PhRosenberger commented Apr 21, 2021

echo_pulse_width added in osi_featuredata.proto for message LidarDetection.

Add a description

As intensity is in % and echo_pulse_width is in m, such a field is currently missing in OSI for LidarDetectionData.
It is necessary to transfer echo_pulse_width since sensors like Ibeo LUX or Valeo SCALA are having this as an output instead of intensity.
The PR comes out of the SET Level project, where it has already been approved by the project-internal OSI-CCB.

Take this checklist as orientation for yourself, if this PR is ready for the Change Control Board:

  • My suggestion follows the style and contributors guidelines.
  • I have taken care about the documentation.
  • I have done the DCO signoff.
  • My changes generate no errors when passing CI tests.
  • I have successfully implemented and tested my fix/feature locally.
  • Appropriate reviewer(s) are assigned.

If you can’t check all of them, please explain why.
If all boxes are checked or commented and you have achieved at least one positive review, you can assign the label ReadyForCCBReview!

@PhRosenberger PhRosenberger requested a review from kmeids April 21, 2021 13:02
@PhRosenberger PhRosenberger self-assigned this Apr 21, 2021
@PhRosenberger PhRosenberger added FeatureRequest Proposals which enhance the interface or add additional features. SensorModeling The Group in the ASAM development project working on sensor modeling topics. labels Apr 21, 2021
@kmeids
Copy link

kmeids commented Apr 30, 2021

@PhRosenberger as discussed in the SM meeting, could you please provide more documentation/explanation on the echo_pulse_width ?

@PhRosenberger
Copy link
Contributor Author

@PhRosenberger as discussed in the SM meeting, could you please provide more documentation/explanation on the echo_pulse_width ?

I have added an explanation with reference to some figures.
@kmeids what do you think about it?

@PhRosenberger PhRosenberger force-pushed the extension_echo-pulse-width branch from f5e970d to 99513b1 Compare May 10, 2021 12:53
@kmeids
Copy link

kmeids commented May 11, 2021

@PhRosenberger I think that is really good!
@max-rosin would it be more practical to have the images Philipp is referring to directly integrated into the documentation and a link to the publication for more info?

@kmeids kmeids added the Documentation Everything which impacts the quality of the documentation and guidelines. label May 11, 2021
@stefancyliax stefancyliax added this to the V3.4.0 milestone Jun 23, 2021
@max-rosin
Copy link
Contributor

From the user perspective, directly integrating the images is better.
However, I do not know whether simply integrating these images produces copyright problems (for example, some sort of exclusivity clause with Springer in the publishing contract).

@PhRosenberger
Copy link
Contributor Author

From the user perspective, directly integrating the images is better.
However, I do not know whether simply integrating these images produces copyright problems (for example, some sort of exclusivity clause with Springer in the publishing contract).

Is this a question to us or are you clarifying it right now?

@max-rosin
Copy link
Contributor

From the user perspective, directly integrating the images is better.
However, I do not know whether simply integrating these images produces copyright problems (for example, some sort of exclusivity clause with Springer in the publishing contract).

Is this a question to us or are you clarifying it right now?

I simply wanted to raise awareness, that we are talking about images which are subject to copyright, publishing contracts, employment contracts etc.
Whoever adds the images to the documentation must also make sure that he or she is allowed to do so and is also capable of granting OSI the permission to re-distribute them under its license. That's basically what the DCO sign-offs on your commits mean. Usually this is not a big deal, but here we are talking about images that have already been published by a big publisher.

But since you asked, I took a second look at the article and saw that it is licensed under Creative Commons Attribution 4.0 International License. This license extends to the images in the article as well, unless otherwise noted. This makes everything a lot easier, because it allows to by-pass many of the more complicated questions.

However, even with creative commons there are still a couple of things to do, namely proper attribution and a link to the license text. I have no idea how to do that properly in the context of OSI.

If complying with the license is infeasible, we can of course always create new images. This has the added benefit that we can improve them and adapt them to OSI.

@kmeids kmeids added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Aug 6, 2021
@stefancyliax
Copy link
Contributor

However, even with creative commons there are still a couple of things to do, namely proper attribution and a link to the license text. I have no idea how to do that properly in the context of OSI.

Clarified this topic inside ASAM:
We can use this image with the footnote:
Figure: Creative Commons from Rosenberger, P., Holder, M.F., Cianciaruso, N. et al. Sequential lidar sensor system simulation: a modular approach for simulation-based safety validation of automated driving. Automot. Engine Technol. 5, 187–197 (2020). Lizenz: CC BY 4.0. Fig 7 / 8 from the original.

Please include @pasched

Comment on lines 454 to 457
// Several sensors output an echo-pulse width instead of an intensity for each single detection.
// It is measured in m and measures the extend of the object parts or atmospheric particles that produce the echo.
// As an example, it is depicted for the two echos reflected from the edges A-B and C-D in Fig. 7 from [Rosenberger et al.](https://link.springer.com/content/pdf/10.1007/s41104-020-00066-x.pdf).
// Fig. 8 [Rosenberger et al.](https://link.springer.com/content/pdf/10.1007/s41104-020-00066-x.pdf) shows it in more detail, as the echo-pulse width is measured as the range between the rising and falling edge crossing the intensity threshold.
Copy link

Choose a reason for hiding this comment

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

Please update the white spaces.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is also the reason the pipeline fails. Patching now.

@kmeids
Copy link

kmeids commented Sep 15, 2021

Output CCB 15.09.2021:

  1. @stefancyliax to take care of adding the pictures referenced by @PhRosenberger.
  2. @pasched to do documentation review and set to "ReadyToMerge".
  3. @pmai to merge after "1" and "2" are done.

@kmeids kmeids removed the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Sep 15, 2021
@stefancyliax
Copy link
Contributor

From the user perspective, directly integrating the images is better.
However, I do not know whether simply integrating these images produces copyright problems (for example, some sort of exclusivity clause with Springer in the publishing contract).

@PhRosenberger Can you provide the images from the linked paper?

@FKlopfer FKlopfer added ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB. and removed ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. labels Sep 17, 2021
@kmeids
Copy link

kmeids commented Oct 13, 2021

CCB Output 13.10.2021:

  1. Update citation according to the following comment (echo_pulse_width added in osi_featuredata.proto for message LidarDetection #511 (comment)).
  2. @pmai to fix the DCO and merge.

@stefancyliax
Copy link
Contributor

Please use this citation style:

[1] Rosenberger, P., Holder, M.F., Cianciaruso, N. et al. Sequential lidar sensor system simulation: a modular approach for simulation-based safety validation of automated driving. Automot. Engine Technol. 5, 187–197 (2020). https://doi.org/10.1007/s41104-020-00066-x

@stefancyliax
Copy link
Contributor

stefancyliax commented Oct 13, 2021

CCB Output 13.10.2021:

  1. Update citation according to the following comment (echo_pulse_width added in osi_featuredata.proto for message LidarDetection #511 (comment)).
  2. @pmai to fix the DCO and merge.

Point 1 done. Over to you @pmai

@pmai pmai force-pushed the extension_echo-pulse-width branch 2 times, most recently from 9b7a4bd to bb85843 Compare October 14, 2021 15:16
Stefan Cyliax and others added 3 commits October 14, 2021 17:17
@pmai pmai force-pushed the extension_echo-pulse-width branch from bb85843 to db8608e Compare October 14, 2021 15:18
@pmai pmai merged commit 3590ea0 into master Oct 14, 2021
@pmai pmai deleted the extension_echo-pulse-width branch October 14, 2021 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Everything which impacts the quality of the documentation and guidelines. FeatureRequest Proposals which enhance the interface or add additional features. ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB. SensorModeling The Group in the ASAM development project working on sensor modeling topics.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants