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

Corrected NTCIP1218 msgRepeatsOption value #68

Closed
wants to merge 2 commits into from

Conversation

dmccoystephenson
Copy link
Member

@dmccoystephenson dmccoystephenson commented Apr 2, 2024

PR Details

Description

Problem

The SnmpNTCIP1218Protocol class is currently using 0x00 for the msg repeats options value, which is incorrect (for the context of deploying an unsigned TIM). It should be using 0x80.

Solution

The value being used for the msg repeats options value in the SnmpNTCIP1218Protocol class has been changed to 0x80.

Related Issue

No related GitHub issue.

Motivation and Context

The SnmpNTCIP1218Protocol class should use the correct value for the msg repeats option value to ensure a successful deposit.

How Has This Been Tested?

Depositing to an RSU has been verified to work with this change.

Types of changes

  • Defect fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that cause existing functionality to change)

Checklist:

  • I have added any new packages to the sonar-scanner.properties file
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
    ODE Contributing Guide
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link

@mwodahl mwodahl 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!

Copy link
Collaborator

@drewjj drewjj left a comment

Choose a reason for hiding this comment

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

It is important to note that x00 isn't incorrect entirely. It is incorrect for the context of deploying an unsigned TIM.

@dmccoystephenson
Copy link
Member Author

It is important to note that x00 isn't incorrect entirely. It is incorrect for the context of deploying an unsigned TIM.

Will deploying a signed TIM using 0x80 work?

@drewjj
Copy link
Collaborator

drewjj commented Apr 16, 2024

It is important to note that x00 isn't incorrect entirely. It is incorrect for the context of deploying an unsigned TIM.

Will deploying a signed TIM using 0x80 work?

If it is deployed to an RSU to be broadcasted from a RSU it will not work. 0x80 tells the RSU to sign the message before broadcasting it. This will cause an already signed message to be doubly wrapped with a 1609.2 header.

In summary:
0x00 - Tells the RSU to broadcast the message without signing it or attaching a 1609.2 header to it.
0x80 - Tells the RSU to sign and attach a 1609.2 header before broadcasting the message.

0xC0 shouldn't be used ever because it achieves some weird hybrid of the two.

@payneBrandon
Copy link

It is important to note that x00 isn't incorrect entirely. It is incorrect for the context of deploying an unsigned TIM.

Will deploying a signed TIM using 0x80 work?

If it is deployed to an RSU to be broadcasted from a RSU it will not work. 0x80 tells the RSU to sign the message before broadcasting it. This will cause an already signed message to be doubly wrapped with a 1609.2 header.

In summary: 0x00 - Tells the RSU to broadcast the message without signing it or attaching a 1609.2 header to it. 0x80 - Tells the RSU to sign and attach a 1609.2 header before broadcasting the message.

0xC0 shouldn't be used ever because it achieves some weird hybrid of the two.

@drewjj do we need a way to configure this then, to allow a choice of whether to have the RSU sign or just forward?

@drewjj
Copy link
Collaborator

drewjj commented Apr 16, 2024

It is important to note that x00 isn't incorrect entirely. It is incorrect for the context of deploying an unsigned TIM.

Will deploying a signed TIM using 0x80 work?

If it is deployed to an RSU to be broadcasted from a RSU it will not work. 0x80 tells the RSU to sign the message before broadcasting it. This will cause an already signed message to be doubly wrapped with a 1609.2 header.
In summary: 0x00 - Tells the RSU to broadcast the message without signing it or attaching a 1609.2 header to it. 0x80 - Tells the RSU to sign and attach a 1609.2 header before broadcasting the message.
0xC0 shouldn't be used ever because it achieves some weird hybrid of the two.

@drewjj do we need a way to configure this then, to allow a choice of whether to have the RSU sign or just forward?

Yeah, that would be ideal if we want the ODE to be flexible. This pr seems like it has half the solution. #67

@dmccoystephenson dmccoystephenson marked this pull request as draft April 30, 2024 19:17
@dmccoystephenson
Copy link
Member Author

Marking this as a draft for now until we modify the functionality to use 0x00 / 0x80 depending on context.

@dmccoystephenson dmccoystephenson deleted the update/1218-msgRepeatValue branch May 3, 2024 22:07
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.

4 participants