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

Add DAY to TrafficSign-Value-Unit and change DAY to WEEKDAY #662

Merged
merged 5 commits into from
Jan 17, 2023
Merged

Add DAY to TrafficSign-Value-Unit and change DAY to WEEKDAY #662

merged 5 commits into from
Jan 17, 2023

Conversation

FlorianMueller87
Copy link
Contributor

@FlorianMueller87 FlorianMueller87 commented Aug 9, 2022 •
edited
Loading

Reference to a related issue in the repository

Issue: #635

Add a description

ASAM OSI and ISO 23150 or AUTOSAR ADI have a common history. Unfortunately, the inner structure, the naming and the definitions of the standards are differentiated from each other. This makes the work of developers unnecessary complicated for mostly no technical reasons. All sides should strive to reduce inequality.

osi_trafficsign.proto message TrafficSignValue { enum Unit } need an entry which describes the day of the month.

Furthermore, the type need a entry kHour which describe the hour of the day.

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.

The checks will follow.

Additional context

ISO23150:2021A.2.67 Sign value unit:

@ThomasNaderBMW @jdsika @schmidtlorenz

@FlorianMueller87 FlorianMueller87 changed the title Add DAY to TrafficSign-Value-Unit and change DAY to WEEKDAY Draft for discussion: Add DAY to TrafficSign-Value-Unit and change DAY to WEEKDAY Aug 9, 2022
@FlorianMueller87 FlorianMueller87 force-pushed the feature/adding_day_of_the_month_to_TrafficSign_Value_Unit branch from 77af07b to bf14727 Compare August 12, 2022 06:47
@jdsika jdsika added the Harmonisation The Group in the ASAM development project working on harmonisation with other standards. label Oct 17, 2022
@jdsika jdsika added this to the V3.6.0 milestone Oct 17, 2022
@jdsika
Copy link
Contributor

jdsika commented Oct 17, 2022 •
edited
Loading

Comments:

  • Renaming a value will break compatibility and is not reasonable if the semantic meaning persists. Reanming it will be done in v4.0.0 in the process of reworking the whole enum
  • In general we have to ask what needs to be achieved: Do we want to add e.g. a full or partial date to a supplementary sign or how do you intend to use this unit? Are we missing then things like "MONTH_OF_YEAR" or "YEAR"?

Current situation:

  • we have "DAY" meaning "day of the week" 0=Monday, and 1=Tuesday. Note: ISO starts with 0=Sunday potentially violating the ISO8601 but starting with 1=Monday. @carsten-kuebler can you have look at this here, please?

What could be done for v3.6.0:

  • add "DAY_OF_MONTH" starting with 1

What could be done for 4.0.0:

  • rename "DAY" to "DAY_OF_WEEK"
  • Let all values start with the value 1 instead of 0 as this is normally done for dates

@jdsika jdsika linked an issue Oct 17, 2022 that may be closed by this pull request
ISO23150 compliance: Adding day of the month to [Traffic-Sign-Value-Unit] #635
Closed
@jdsika
Copy link
Contributor

jdsika commented Nov 23, 2022 •
edited
Loading

Comments:

  • Duration (missing) vs time of day
    • UNIT_DURATION_MINUTE
    • UNIT_DURATION_HOUR
    • UNIT_DURATION_DAY
  • Use singular and not plural: v4.0
    • UNIT_MINUTES to UNIT_MINUTE (comment is wrong -> BUG)
    • UNIT_FEET to UNIT_FOOT
  • Add comment referencing ISO8601 and add e.g. that minute and hour start with 0

@jdsika
Copy link
Contributor

jdsika commented Nov 23, 2022

Proposal:

  • PR für v3.6
  • Issue für v4.0

@jdsika jdsika assigned jdsika and unassigned FlorianMueller87 Nov 23, 2022
@jdsika jdsika requested a review from pmai November 23, 2022 10:26
@jdsika jdsika mentioned this pull request Nov 23, 2022
Open
@jdsika
Copy link
Contributor

jdsika commented Nov 23, 2022

@pmai please review and set READY_FOR_CCB afterwards. Sorry for messing up the DCO.

@thempen thempen requested a review from PhRosenberger December 7, 2022 09:50
@jdsika jdsika changed the title Draft for discussion: Add DAY to TrafficSign-Value-Unit and change DAY to WEEKDAY Add DAY to TrafficSign-Value-Unit and change DAY to WEEKDAY Dec 7, 2022
@pmai pmai force-pushed the feature/adding_day_of_the_month_to_TrafficSign_Value_Unit branch from c994746 to 6bafb05 Compare December 7, 2022 10:00
pmai
pmai approved these changes Dec 7, 2022
View reviewed changes
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.

Looks good to me.

@thempen thempen added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Dec 7, 2022
@pmai pmai 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 Jan 16, 2023
@pmai
Copy link
Contributor

pmai commented Jan 16, 2023

CCB 2023-01-16: Merge as-is.

@FlorianMueller87 @pmai
Add DAY to TrafficSign-Value-Unit and change DAY to WEEKDAY
3769237 
Issue: OpenSimulationInterface#635
Signed-off-by: FlorianMueller87 <florian.b.mueller@gmx.de>
@FlorianMueller87 @pmai
Add HOUR to TrafficSignsValue-Unit
9b64ec2 
Issue: OpenSimulationInterface#635
Signed-off-by: FlorianMueller87 <florian.b.mueller@gmx.de>
@FlorianMueller87 @pmai
Change description of UNIT_MINUTES
3159890 
Issue: OpenSimulationInterface#638
Signed-off-by: FlorianMueller87 <florian.b.mueller@gmx.de>
FlorianMueller87 and others added 2 commits January 16, 2023 12:34
@FlorianMueller87 @pmai
Change value of UNIT_HOUR from 15 to 16
399e5b0 
Signed-off-by: FlorianMueller87 <florian.b.mueller@gmx.de>
@jdsika @pmai
Added changes for v3.6.0
96f9f48 
* Added DURATION_*
* Added UNIT_DAY_OF_MONTH
* Added UNIT_HOUR for time of day
* Fixed bug in comment for UNIT_MINUTES

Signed-off-by: Carlo van Driesten <carlo.van-driesten@bmw.de>
@pmai pmai force-pushed the feature/adding_day_of_the_month_to_TrafficSign_Value_Unit branch from 6bafb05 to 96f9f48 Compare January 16, 2023 11:34
@pmai pmai merged commit b13e49c into OpenSimulationInterface:master Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmai pmai pmai approved these changes

@PhRosenberger PhRosenberger PhRosenberger approved these changes

Assignees

@jdsika jdsika

@PhRosenberger PhRosenberger

Labels
Harmonisation The Group in the ASAM development project working on harmonisation with other standards. ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB.
Projects
None yet
Milestone
V3.6.0
Development

Successfully merging this pull request may close these issues.

ISO23150 compliance: Adding day of the month to [Traffic-Sign-Value-Unit]
5 participants
@FlorianMueller87 @jdsika @pmai @PhRosenberger @thempen