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

schema/cam update schema according to CAM ETSI TS 103 900 V2.1.1 #248

Merged

Conversation

Hugues360
Copy link
Collaborator

@Hugues360 Hugues360 commented Dec 13, 2024

Features:

  • Update CAM schema according to CAM ETSI TS 103 900 V2.1.1

Closes: #217


How to test:

  1. Check the workflow job for CAM schema 2.1.0

  2. Compare the 2.1.0 schema to the CAM ETSI TS 103 900 V2.1.1 (https://forge.etsi.org/rep/ITS/asn1/cam_ts103900/-/tree/V2.1.1)


Expected results:

  1. The job succeeds (no validation error occcurs)

  2. The 2.1.0 CAM schema should correspond to the Technical Specification

Signed-off-by: Hugues Oudeville <hugues.oudeville@orange.com>
@Hugues360 Hugues360 self-assigned this Dec 13, 2024
Copy link
Member

@ymorin-orange ymorin-orange left a comment

Choose a reason for hiding this comment

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

Can you please split the big commit into a set of more digestible commits, for example:

  • 1 commit to fix the existing schema without adding anything new
  • 1 commit adding the RSU container
  • 1 commit adding the special_vehicle_container with (e.g.) just public_transport_container
  • 1 commit for each of the other special_vehicle_container alternatives...

@Hugues360 Hugues360 force-pushed the houdeville/update_cam_schema branch from 50dbcf7 to 4a945c7 Compare December 17, 2024 14:07
@ymorin-orange
Copy link
Member

Thanks for the quick respin! :-) I'll look into it tomorrow morning...

In the meantime, a few comments about the commit log titles (cf process documentation being reviewd internally):

  • missing colon in title for commit 2: schema/cam: add new references field
  • extraneous space after colon in most commit titles
  • use imperative form: s/adding/add/ and s/updating/update/
  • type: s/udating/update/
  • warp long lines at ~72 chars

@Hugues360 Hugues360 force-pushed the houdeville/update_cam_schema branch 2 times, most recently from c343c38 to 480ab1e Compare December 18, 2024 07:24
@ymorin-orange ymorin-orange self-requested a review December 18, 2024 12:31
Copy link
Member

@ymorin-orange ymorin-orange left a comment

Choose a reason for hiding this comment

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

If you agree with the proposal by @mathieu1fb:

  • objects that have a _value and a _confidence properties, and when both those properties have an unavalaible value, then:
    • that object is not required
    • the _value property for that object is required
    • the _confidence property for that object is not required
    • do not add any $comment (but keep existing ones to avoid churn)
    • also, it's JSON, so ordering of keys in objects does not matter; where possible, I think the confidence property of an object should be after all the other properties
    • For example, the altitude object in reference_position:
      "reference_position": {
          "type": "object",
          "required": [
              "latitude",
              "longitude"
          ],
          "latitude": {...},
          "longitude": {...},
          "altitude": {
              "type": "object",
              "required": [
                  "altitude_value"
              ],
              "altitude_value": {
                  "type": "integer",
                  "description": "...",
                  "default": 800001,
                  "minimum": -100000,
                  "maximum": 800001
              },
              "altitude_confidence": {
                  "type": "integer",
                  "description": "...",
                  "default": 15,
                  "minimum": 0,
                  "maximum": 15
              }
          },
          "position_confidence_elipse": {...}
      }

@Hugues360
Copy link
Collaborator Author

Hugues360 commented Dec 18, 2024

If you agree with the proposal by @mathieu1fb:

* objects that have a `_value` and a `_confidence` properties, and when both those properties have an `unavalaible` value, then:
  
  * that object is not `required`
  * the `_value` property for that object is required
  * the `_confidence` property for that object is not required
  * do not add any `$comment` (but keep existing ones to avoid churn)
  * also, it's JSON, so ordering of keys in objects does not matter; where possible, I think the _confidence_ property of an object should be after all the other properties
  * For example, the `altitude` object in `reference_position`:
    ```json
    "reference_position": {
        "type": "object",
        "required": [
            "latitude",
            "longitude"
        ],
        "latitude": {...},
        "longitude": {...},
        "altitude": {
            "type": "object",
            "required": [
                "altitude_value"
            ],
            "altitude_value": {
                "type": "integer",
                "description": "...",
                "default": 800001,
                "minimum": -100000,
                "maximum": 800001
            },
            "altitude_confidence": {
                "type": "integer",
                "description": "...",
                "default": 15,
                "minimum": 0,
                "maximum": 15
            }
        },
        "position_confidence_elipse": {...}
    }
    ```

Okay. I agree. I am going to update according to this proposal

But @ymorin-orange, @mathieu1fb just to get sure before starting :

  • this apply only for objects that have a _value and a _confidence properties: for example drive_direction remains required even if he has an unavailable value? Idem for all the fields of position_confidence_ellipse that will remain required? Or we extend this to all the fields that have a unavailable value?
  • You asked to leave the existing $comment but as I did not add any , I leave all of them?

@ymorin-orange
Copy link
Member

  • this apply only for objects that have a _value and a _confidence properties: for example drive_direction remains required even if he has an unavailable value? Idem for all the fields of position_confidence_ellipse that will remain required? Or we extend this to all the fields that have a unavailable value?

IMHO, I would say it should also apply to any property that has an unavailable value, in fact. @mathieu1fb OK?

  • You asked to leave the existing $comment but as I did not add any , I leave all of them?

Then drop the comments in the objects/properties you move around or otherwise fix. And you did add at least one, in basic_vehicle_container_high_frequence.performance_class (im commit "schema/cam: update high frequency container for vehicle"), for example...

@mathieu1fb
Copy link
Collaborator

mathieu1fb commented Dec 18, 2024

  • this apply only for objects that have a _value and a _confidence properties: for example drive_direction remains required even if he has an unavailable value? Idem for all the fields of position_confidence_ellipse that will remain required? Or we extend this to all the fields that have a unavailable value?

IMHO, I would say it should also apply to any property that has an unavailable value, in fact. @mathieu1fb OK?

I'd say OK, unless it turns out absolutely every field is optional and we cannot make sense of the message that would be sent (let's say a CAM without latitude and longitude...).

Unless it allows to send lighter CAMs with only what has changed compared to the previous one!

@ymorin-orange
Copy link
Member

  • this apply only for objects that have a _value and a _confidence properties: for example drive_direction remains required even if he has an unavailable value? Idem for all the fields of position_confidence_ellipse that will remain required? Or we extend this to all the fields that have a unavailable value?

IMHO, I would say it should also apply to any property that has an unavailable value, in fact. @mathieu1fb OK?

I'd say OK, unless it turns out absolutely every field is optional and we cannot make sense of the message that would be sent (let's say a CAM without latitude and longitude...).

Yes, there are fields that are really mandatory! Latitude and longitude are, for sure!

Unless it allows to send lighter CAMs with only what has changed compared to the previous one!

Unlike DENM, I don't think there is a sequence number in a CAM, so we would not know which previous one to update (but a given object may only have a single position anyway. Let's not make that too complex either!)...

@Hugues360
Copy link
Collaborator Author

Then drop the comments in the objects/properties you move around or otherwise fix. And you did add at least one, in basic_vehicle_container_high_frequence.performance_class (im commit "schema/cam: update high frequency container for vehicle"), for example...

You are right. My bad.

@Hugues360 Hugues360 force-pushed the houdeville/update_cam_schema branch from 480ab1e to 5152103 Compare December 18, 2024 21:41
Signed-off-by: Hugues Oudeville <hugues.oudeville@orange.com>
Signed-off-by: Hugues Oudeville <hugues.oudeville@orange.com>
Signed-off-by: Hugues Oudeville <hugues.oudeville@orange.com>
@Hugues360 Hugues360 force-pushed the houdeville/update_cam_schema branch from 5152103 to 93628fc Compare December 20, 2024 07:52
},
"cen_dsrc_tolling_zone": {
"type": "object",
"description": "information about a the position of a CEN DRSC Tolling Station operating in the 5,8 GHz frequency band.",
Copy link
Member

Choose a reason for hiding this comment

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

Typo: s/DRSC/DSRC/ (ref)

Copy link
Member

@ymorin-orange ymorin-orange left a comment

Choose a reason for hiding this comment

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

Except for the minor typo, it all seems good to me, now. When you fix that typo and re-push, just merge without requesting another review.

@Hugues360
Copy link
Collaborator Author

Hugues360 commented Dec 20, 2024

Except for the minor typo, it all seems good to me, now. When you fix that typo and re-push, just merge without requesting another review.

Ok, typo fixed, and thank you for the review.

Signed-off-by: Hugues Oudeville <hugues.oudeville@orange.com>
Signed-off-by: Hugues Oudeville <hugues.oudeville@orange.com>
In CAM TS 103 900 v2.1.1, the high frequency container can be either a basic vehicle high frequency container or a RSU high frequency container

Signed-off-by: Hugues Oudeville <hugues.oudeville@orange.com>
Signed-off-by: Hugues Oudeville <hugues.oudeville@orange.com>
Signed-off-by: Hugues Oudeville <hugues.oudeville@orange.com>
Signed-off-by: Hugues Oudeville <hugues.oudeville@orange.com>
Signed-off-by: Hugues Oudeville <hugues.oudeville@orange.com>
Signed-off-by: Hugues Oudeville <hugues.oudeville@orange.com>
Signed-off-by: Hugues Oudeville <hugues.oudeville@orange.com>
Signed-off-by: Hugues Oudeville <hugues.oudeville@orange.com>
Signed-off-by: Hugues Oudeville <hugues.oudeville@orange.com>
@Hugues360 Hugues360 force-pushed the houdeville/update_cam_schema branch from 93628fc to d01d821 Compare December 20, 2024 09:54
@Hugues360 Hugues360 merged commit e61a3dc into Orange-OpenSource:master Dec 20, 2024
58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review CAM schema against ETSI spec
3 participants