Skip to content

Extension by HMI-, ADAS- and Dynamic-Values - #286

Closed
ThomasNaderBMW wants to merge 43 commits into
OpenSimulationInterface:masterfrom
ThomasNaderBMW:OSI_Extension_HMI_ADAS_Dynamic
Closed

Extension by HMI-, ADAS- and Dynamic-Values#286
ThomasNaderBMW wants to merge 43 commits into
OpenSimulationInterface:masterfrom
ThomasNaderBMW:OSI_Extension_HMI_ADAS_Dynamic

Conversation

@ThomasNaderBMW

@ThomasNaderBMWThomasNaderBMW commented Oct 24, 2018

Copy link
Copy Markdown
Contributor

Think about a simulation-construct consisting of a human driver, an AD-function and also a dynamic-model.
All these pieces can come from different authors, but should work together.
This is a requirement the driving-simulation-sector has to deal with:

A test person wants to activate an external highly automated driving function in a mock-up. So the function has to listen to the mock-up. The other way around the mock-up has to understand commands from the function (Take-over-request?). So this is an interface that should be standardized.
Furthermore, it is the same with an external dynamic-model. How can this listen to the simulation-environment to get values like the friction of the street needed for dynamic-calculations? And also here, the other way around: The results of the dynamic-model describe the state of a vehicle regarding kinematics. Additionally, calculated values for the engine or the steering can be part of. Both datasets are worth to be standardized.

Dear OSI-Community, please help to enhance the appended .proto-signals.

Think about a simulation-construct consisting of a human driver, an AD-function and also a dynamic-model.
All these pieces can come from different authors, but should work together.
This is a requirement the driving-simulation-sector has to deal with:
A test person wants to activate an external highly automated driving in a mock-up. So the function has to listen to the mock-up. The other way around the mock-up has to understand commands from the function (Take-over-request?). So this is an interface that should be standardized.
Furthermore, it is the same with an external dynamic-model. How can this listen to the simulation-environment to get values like the friction of the street needed for dynamic-calculations? And also here, the other way around: The results of the dynamic-model describe the state of a vehicle regarding kinematics. Additionally, calculated values for the engine or the steering can be part of. Both datasets are worth to be standardized.
Dear OSI-Community, please help to enhance the appended .proto-signals.
@mohammadbahrammohammadbahram added FeatureRequest Proposals which enhance the interface or add additional features. HelpWanted I have tried my best - please help me! Suggestions I just want to drop by and leave this suggestion to think about. labels Oct 24, 2018
@mohammadbahrammohammadbahram added this to the v3.1.0 milestone Oct 24, 2018
@mohammadbahram
mohammadbahram requested review from a user and pmaiOctober 24, 2018 16:00
@mohammadbahrammohammadbahram removed the HelpWanted I have tried my best - please help me! label Oct 24, 2018
@ghost

Copy link
Copy Markdown

@ThomasNaderBMW You can access the build log in order to check why it is not building. It is basically missing comments and tabs which have to be replaced with spaces.

@pmai

pmai commented Oct 29, 2018

Copy link
Copy Markdown
Contributor

Before commenting on specifics on this proposal, it would really help if there was some short overview of the architectural intent, i.e. which models are involved, and which messages do they receive and send. This is also something we should add to the documentation once the PR is merged, since these are very common questions in how to use OSI.

@mohammadbahram

Copy link
Copy Markdown

@ThomasNaderBMW could you please add a short overview of the architectural intent?

@ThomasNaderBMW

Copy link
Copy Markdown
ContributorAuthor

Of course, I made a short overview. Attached is the requested architectural intent:
OSI_Extension_Architectural_Intent.pdf

@ghost

Copy link
Copy Markdown

@CezarySzyszkaAltran do you have time to review this?

  • consistent names and messages
  • the osi_common_extension has to be integrated in osi_common, what do you think?
  • are the names generic enough?
  • do we have duplicated information: light states e.g.?
  • etc

@CezarySzyszkaAltranCezarySzyszkaAltran left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Main Problem

The OSI is data serialization protocol. I am not sure it is if official but data serialized to string can be only of type SensorData or SensorView. All objects and states described in simulated environment have some place in tree like structure. For example SensorView.global_ground_truth.moving_object[].base.position.x is a place to put x coordinate of a vehicle. The messages here are detached from mentioned tree. This is not acceptable. We need to find a place to attach those information to the.

Maybe creation of osi_egocar.proto would be a good idea. The global_ground_truth can have ego car as as special field. This would be a major change to the standard and I would not recommend it for release 3.1.

Naming

  • do not use plural in osi
  • separate words with undescore
  • state bool variables as clear question

osi_common

This is a collection of types used across many different derived types.

Wheels and VehicleWheels

Those two classes are fragile in design. The issue is explained in-line

Comment threadosi_vehicle.proto Outdated
Comment threadosi_vehicle.proto Outdated
Comment threadosi_environment_extension.proto Outdated
Comment threadosi_environment_extension.proto Outdated
Comment threadosi_environment_extension.proto
Comment threadosi_common_extension.proto
Comment threadosi_driver_inputs.proto Outdated
Comment threadosi_vehicle.proto Outdated
Comment threadosi_vehicle.proto Outdated
Comment threadosi_adas_function.proto Outdated
@ThomasNaderBMW

Copy link
Copy Markdown
ContributorAuthor

Thanks so far to @ppannusch and @CezarySzyszkaAltran .
You gave a lot and important input!

Alltogether we discussed some different points, at many I don't know what is the best way.
So now and if we want to merge to OSI 3.1 in a few days, I "give the ball" to @CarloVanDriestenBMW and @pmai to make the best out of the suggestions.
Otherwise, so if some time is left in the new year, we can discuss this together.

@ghostghost modified the milestones: v3.1.0, v3.2.0Dec 21, 2018
@ThomasNaderBMW

Copy link
Copy Markdown
ContributorAuthor

So now I corrected a lot after talking to Cezary.
@CezarySzyszkaAltran : Can you do the review again?

@ghostghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please have a look at the two comments regarding the comment style. Consider creating a branch in the OSI repository instead of using your fork in order for me to make changes directly. Close the PR and open a new branch.

//
message Steeringwheel
{
// Angle of the steeringwheel.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

// Angle of the steeringwheel. //
// Unit: [rad]
//
// \note angle < 0 := left; angle = 0 := Central(Straight); angle > 0 := right;
//

//
message Pedalry
{
// Position of the acceleration-pedal.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

// Position of the acceleration-pedal.
//
// Unit: [ ]
//
// Range: [0.0, 1.0]
//
// \note 0.0 := unpressed; 1.0 := fully pressed
//

@ghost

ghost commented Mar 5, 2019

Copy link
Copy Markdown

@ThomasNaderBMW when openeing a new PR through a branch could you please summarize quickly which suggestions you already incorporated and what issues are up for discussion? Changes have been made but have not been "resolved officially" which makes the tracking hard.

@ThomasNaderBMW

Copy link
Copy Markdown
ContributorAuthor

I opened a new PR as branch as wished. This one can be closed. Thanks so far to all!

@ghost

Copy link
Copy Markdown

This is consolidated in #309

@ghostghost closed this Mar 12, 2019
@jdsikajdsika removed this from the v3.2.0 milestone Feb 19, 2020
This pull request was closed.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

FeatureRequestProposals which enhance the interface or add additional features.SuggestionsI just want to drop by and leave this suggestion to think about.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@ThomasNaderBMW@pmai@mohammadbahram@ppannusch-bmw@CezarySzyszkaAltran@jdsika