Skip to content

Add automated driving function state to host vehicle data #589

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 7 commits into from
Feb 4, 2022

Conversation

caspar-ai
Copy link
Contributor

@caspar-ai caspar-ai commented Oct 13, 2021

Reference to a related issue in the repository

#545

Add a description

See the linked issue for more details, but at a high level, this adds the ability for a host vehicle to report details about the internal state of automated driving functions.

It also adds HostVehicleData to a TrafficUpdate to allow this information to be reported from a SuT to the simulator, or other simulation entities.

(HostVehicleData is already a top level message, and embedded in other "reporting" type messages, to allow this information to be propagated further if necessary)

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.

@caspar-ai caspar-ai requested a review from pmai October 13, 2021 14:30
@caspar-ai caspar-ai self-assigned this Oct 13, 2021
Copy link
Contributor

@pmai pmai left a comment

Choose a reason for hiding this comment

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

I think this is the right approach and is already looking very good to me, with a couple of suggested changes/clarifications below. With those fixed it should theoretically be good to go from my point of view. But more airing should be given to other people involved in HostVehicleData.

@pmai pmai requested a review from ThomasNaderBMW October 14, 2021 09:53
@pmai
Copy link
Contributor

pmai commented Oct 14, 2021

@ThomasNaderBMW this proposal for the driver assistance information to be added to HostVehicleData seems pretty good to me, could you have a look?

At least the addition of the ID field (since we are making HostVehicleData a proper top-level message this is likely required for disambiguation) and potentially the optional addition to TrafficUpdate as a feedback mechanism to the simulator (e.g. for inclusion in SensorView) should likely go into 3.4 already, with the rest then added in 3.5 quickly thereafter, if not into 3.4...

@caspar-ai
Copy link
Contributor Author

Thanks! Comments all make sense. I'll look at fixing those up today or tomorrow.

@caspar-ai caspar-ai mentioned this pull request Oct 15, 2021
6 tasks
@caspar-ai
Copy link
Contributor Author

Thanks @pmai, I've made all the changes you suggested.

I will leave this as draft until #547 has been merged then I can rebase this onto master and mark as ready for review.

@ThomasNaderBMW
Copy link
Contributor

Good PR @caspar-ai !! Many good ideas we should discuss next CCB.
As stated in the comments I would appreciate merging my PR first and then do a rebase to the master here.
As general inspiration for your content, it would be good if you take a look at
https://github.com/OpenSimulationInterface/open-simulation-interface/pull/350/files
Naming, structuring and so on. I don't say it's better, but maybe contains some improvements ;)

@pmai pmai force-pushed the Extension_osi_hostvehicledata_v2 branch 2 times, most recently from 2795866 to 41946d4 Compare October 19, 2021 10:13
Base automatically changed from Extension_osi_hostvehicledata_v2 to master October 21, 2021 13:23
@caspar-ai caspar-ai force-pushed the 545-add-driver-assist-to-host-vehicle-data branch from d4d5c1c to 69e08f0 Compare October 21, 2021 14:43
@caspar-ai
Copy link
Contributor Author

@ThomasNaderBMW I've had another look through the changes in #350, as a result I have:

  • used the more generic message name from that PR for the message (VehicleAutomatedDrivingFunction)
  • updated all the wording throughout from feature to function to match
  • merged the two lists of function enums as best seemed sensible to me, but happy to change
    • added both references to the comment at the top
  • added the UNAVAILABLE_STATE as that seemed relevant, I couldn't immediately work out what STARTING and STOPPING would be used for so didn't add, but happy to add in if they are useful

In terms of what I didn't do:

  • I've not taken the extra intention and external requests fields
    • I do think these are really useful additions, but I also think they are don't have to be coupled with the basic name / state that is there now
    • as such, I thought it might be easiest to leave out for now to simplify discussions, and we can add in afterwards in a new PR
  • I haven't included LEVEL_1, LEVEL_2 in the names of the functions
    • overall with the names, I already worry that there isn't a good set of common terms
    • adding in the level to the name feels like it adds more complexity to that
    • if we want to capture the level, perhaps that would be best as a separate field on the message? This would potentially allow a clearer distinction
    • for example, the same function PARKING_ASSIST could be level 1, 2, or 4 depending on the sophistication, this would remove the need for using _VALET and _ASSISTANT to distinguish, which sound potentially like company-specific names used to identify them
    • (admittedly it does break down in some cases, by the same logic CRUISE_CONTROL and ADAPTIVE_CRUISE_CONTROL could be the same function name at different levels, but both are well recognised industry terms)
    • one for discussion I think!

Hope that all makes sense, let me know what you think.

@caspar-ai caspar-ai changed the title Add driver assist to host vehicle data Add automated driving function state to host vehicle data Oct 22, 2021
@caspar-ai caspar-ai marked this pull request as ready for review October 22, 2021 17:06
@caspar-ai caspar-ai added FeatureRequest Proposals which enhance the interface or add additional features. ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. labels Oct 25, 2021
@ThomasNaderBMW
Copy link
Contributor

@ThomasNaderBMW I've had another look through the changes in #350, as a result I have:

  • used the more generic message name from that PR for the message (VehicleAutomatedDrivingFunction)

  • updated all the wording throughout from feature to function to match

  • merged the two lists of function enums as best seemed sensible to me, but happy to change

    • added both references to the comment at the top
  • added the UNAVAILABLE_STATE as that seemed relevant, I couldn't immediately work out what STARTING and STOPPING would be used for so didn't add, but happy to add in if they are useful

In terms of what I didn't do:

  • I've not taken the extra intention and external requests fields

    • I do think these are really useful additions, but I also think they are don't have to be coupled with the basic name / state that is there now
    • as such, I thought it might be easiest to leave out for now to simplify discussions, and we can add in afterwards in a new PR
  • I haven't included LEVEL_1, LEVEL_2 in the names of the functions

    • overall with the names, I already worry that there isn't a good set of common terms
    • adding in the level to the name feels like it adds more complexity to that
    • if we want to capture the level, perhaps that would be best as a separate field on the message? This would potentially allow a clearer distinction
    • for example, the same function PARKING_ASSIST could be level 1, 2, or 4 depending on the sophistication, this would remove the need for using _VALET and _ASSISTANT to distinguish, which sound potentially like company-specific names used to identify them
    • (admittedly it does break down in some cases, by the same logic CRUISE_CONTROL and ADAPTIVE_CRUISE_CONTROL could be the same function name at different levels, but both are well recognised industry terms)
    • one for discussion I think!

Hope that all makes sense, let me know what you think.

@caspar-ai : Wow good job and glad to hear you took out some suggestions!
To your points, beside the comments I made:

  • I agree on STARTING and STOPPING are not needed, at least in the first step
  • Regarding states and intentions we also need a discussion in the CCB. If we keep it open intentions may be added in the future, we should already now adapt the structure to keep it extendible
  • Regarding the LEVEL_x assignment I see your point. Maybe it is better to leave it open how advanced the function is.

@kmeids kmeids added this to the V3.5.0 milestone Nov 24, 2021
@kmeids
Copy link

kmeids commented Dec 8, 2021

Output CCB 08.12.2021:

  1. @ThomasNaderBMW could you please review the final updates and confirm if everything is OK.
  2. @everyone, please perform a final documentation review.

@kmeids kmeids added the Documentation Everything which impacts the quality of the documentation and guidelines. label Dec 8, 2021
@kmeids
Copy link

kmeids commented Jan 19, 2022

Output from CCB 19.01.2022:

  1. @ThomasNaderBMW to directly contact @caspar-ai to discuss the following: Add automated driving function state to host vehicle data #589 (comment)

@kmeids kmeids removed the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Jan 19, 2022
Signed-off-by: Caspar de Haes <caspar.dehaes@five.ai>
Signed-off-by: Caspar de Haes <caspar.dehaes@five.ai>
Signed-off-by: Caspar de Haes <caspar.dehaes@five.ai>
Signed-off-by: Caspar de Haes <caspar.dehaes@five.ai>
@caspar-ai caspar-ai force-pushed the 545-add-driver-assist-to-host-vehicle-data branch from 2c5e26d to afc494c Compare January 20, 2022 15:27
To remove complexity and confusion from the state enum.

Also remove redundant prefixes for fields and enums.

Signed-off-by: Caspar de Haes <caspar.dehaes@five.ai>
@caspar-ai caspar-ai force-pushed the 545-add-driver-assist-to-host-vehicle-data branch from afc494c to f193bc8 Compare January 20, 2022 15:43
@stefancyliax stefancyliax added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Feb 2, 2022
@stefancyliax
Copy link
Contributor

CCB 02.02.2022:

  • Decision: Topic can be merged once two small issues still open are fixed (DCO, Reference style).
  • Merge to be done by: @stefancyliax

Copy link
Contributor

@ThomasNaderBMW ThomasNaderBMW left a comment

Choose a reason for hiding this comment

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

As discussed in CCB.

@kmeids kmeids added 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 Feb 2, 2022
Signed-off-by: Habedank Clemens <qxs2704@europe.bmw.corp>
@clemenshabedank clemenshabedank force-pushed the 545-add-driver-assist-to-host-vehicle-data branch from 7f13711 to 7ecdf36 Compare February 2, 2022 12:39
Signed-off-by: Stefan Cyliax <stefan.cyliax@asam.net>
@stefancyliax stefancyliax merged commit ebf8be2 into master Feb 4, 2022
@stefancyliax stefancyliax deleted the 545-add-driver-assist-to-host-vehicle-data branch February 4, 2022 07:56
adrianschultz pushed a commit to adrianschultz/open-simulation-interface that referenced this pull request May 25, 2022
…tionInterface#589)

* Add driver assist to host vehicle data

Signed-off-by: Caspar de Haes <caspar.dehaes@five.ai>

* Feedback from review

Signed-off-by: Caspar de Haes <caspar.dehaes@five.ai>

* Make the structure more generic to all automated driving

Signed-off-by: Caspar de Haes <caspar.dehaes@five.ai>

* Code review mark-ups

Signed-off-by: Caspar de Haes <caspar.dehaes@five.ai>

* Split out driver override

To remove complexity and confusion from the state enum.

Also remove redundant prefixes for fields and enums.

Signed-off-by: Caspar de Haes <caspar.dehaes@five.ai>

* small change

Signed-off-by: Habedank Clemens <qxs2704@europe.bmw.corp>

* Doc: fix reference style for links

Signed-off-by: Stefan Cyliax <stefan.cyliax@asam.net>

Co-authored-by: clemenshabedank <clemens.habedank@partner.bmw.de>
Co-authored-by: Stefan Cyliax <stefan.cyliax@asam.net>
Signed-off-by: Adrian Vernickel <adrian.vernickel@hexagon.com>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants