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

Feature/pnc related #44

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Feature/pnc related #44

wants to merge 3 commits into from

Conversation

a-w50
Copy link
Contributor

@a-w50 a-w50 commented Nov 9, 2024

Describe your changes

See commit messages

Issue ticket number and link

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have made corresponding changes to the documentation
  • I read the contribution documentation and made sure that my changes meet its requirements

a-w50 added 2 commits November 9, 2024 11:55
- variant for `authorization_mode` in `AuthorizationReq` has been
  introduced
- sdp packet buffer size has been increased to 4k, in order to be large
  enough to contain sample data from josev
- crucial bug has been fixed in `ServiceDetail` serialization, where the
  `ParameterSet` has not been initialized/zero-initialized properly --
  this will need some follow up discussion
- some PnC related message and state handling has been added/refactored
- minor refactorings on conversion functions

Signed-off-by: aw <aw@pionix.de>
- some comments have been added, which should be handled in near future

Signed-off-by: aw <aw@pionix.de>
Signed-off-by: aw <aw@pionix.de>
@SebaLukas SebaLukas self-assigned this Nov 13, 2024
Copy link
Collaborator

@SebaLukas SebaLukas left a comment

Choose a reason for hiding this comment

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

This looks good as preparation for the PnC implementation 👍

I would suggest that you change the PR title. With the current title one could think that the complete PnC implementation will take place here and then PnC will work. Which is not the case.

uint8_t buffer[2048];
// FIXME (aw): what buffer size to allow here? Could also be made
// dynamic
uint8_t buffer[4096];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should start to use std::array instead of a c buffer.
I am fine with increasing the buffer size to 4096. I think that's more of an experience value. 4096 should be enough for now.

@@ -17,6 +17,7 @@ namespace feedback {

enum class Signal {
REQUIRE_AUTH_EIM,
REQUIRE_AUTH_PNC,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here we should send also the contract cert chain from the AuthorizationReq message as well. But only once.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This does not have to be part of this PR, but will be added when PnC is fully implemented.

if (in.discharge_limits.has_value()) {
if (not in.discharge_limits) {
return;
}
auto& discharge_limits = in.discharge_limits.value();
Copy link
Collaborator

Choose a reason for hiding this comment

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

const auto instead of auto

@@ -77,8 +88,7 @@ FsmSimpleState::HandleEventReturnType AuthorizationSetup::handle_event(Allocator
return sa.PASS_ON;
}

// Todo(sl): PnC is currently not supported
ctx.feedback.signal(session::feedback::Signal::REQUIRE_AUTH_EIM);
ctx.feedback.signal(required_auth_signal(res));
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should move this feedback.signal call to the authorization state. Only in the authorization state is it clear whether the car is requesting PnC or EIM.

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.

2 participants