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

fix FormationViolation to FormatViolation on CallError (OCPP 2.0.1 Specification, Table B06.FR.17) #202

Closed
wants to merge 5 commits into from

Conversation

dwibudut
Copy link
Contributor

@dwibudut dwibudut commented Jun 13, 2023

FormationViolation to FormatViolation base on OCPP 2.0.1 Specification Table B06.FR.17

@dwibudut
Copy link
Contributor Author

dwibudut commented Jun 13, 2023

Hello,

I just fix the GetInstalledCertificateIdsRequest and GetInstalledCertificateIdsResponse features based on json schema, I've also modified the test function.

Changes:

  • Modify field certificateType on GetInstalledCertificateIdsRequest to array type not string (base on json schema)
  • Add field certificateHashDataChain that contain array of certificateHashData on GetInstalledCertificateIdsResponse (base on json schema)

Thx

@dwibudut
Copy link
Contributor Author

dwibudut commented Jun 15, 2023

I added another fix on typeOfCertificate to certificateType according to json schema in CertificateSignedRequest feature

Thx

@dwibudut dwibudut changed the title fix FormationViolation to FormatViolation on CallError fix FormationViolation to FormatViolation on CallError (OCPP 2.0.1 Specification, Table B06.FR.17) Jun 22, 2023
Copy link
Owner

@lorenzodonini lorenzodonini left a comment

Choose a reason for hiding this comment

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

First of all thanks a lot for finding and fixing these implementation errors.

All commits except the first one LG2M (I left one minor comment).

Regarding the FormatViolation I cannot accept the change the way it is, because in v1.6 the error code is indeed called FormationViolation (for whatever reason this was changed in v2). I believe this might just be the only change between the two ocppj versions. Fixing one version would break the other. To not break anything we probably need to inject error codes from the top layer. I would suggest splitting this into 2 separate PRs.

@@ -284,7 +284,7 @@ type CSMS interface {
// Retrieves all messages currently configured on a charging station.
GetDisplayMessages(clientId string, callback func(*display.GetDisplayMessagesResponse, error), requestId int, props ...func(*display.GetDisplayMessagesRequest)) error
// Retrieves all installed certificates on a charging station.
GetInstalledCertificateIds(clientId string, callback func(*iso15118.GetInstalledCertificateIdsResponse, error), typeOfCertificate types.CertificateUse, props ...func(*iso15118.GetInstalledCertificateIdsRequest)) error
GetInstalledCertificateIds(clientId string, callback func(*iso15118.GetInstalledCertificateIdsResponse, error), certificateTypes []types.CertificateUse, props ...func(*iso15118.GetInstalledCertificateIdsRequest)) error
Copy link
Owner

Choose a reason for hiding this comment

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

In theory the certificateTypes are optional so they could be omitted from this signature (all optional values are ommitted throughout the lib as they can be added via the props variadic function). That being sad, if this leads to too many changes, we could also leave it as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello, I've modified these changes based on your review and created a new PR on #207

@dwibudut
Copy link
Contributor Author

First of all thanks a lot for finding and fixing these implementation errors.

All commits except the first one LG2M (I left one minor comment).

Regarding the FormatViolation I cannot accept the change the way it is, because in v1.6 the error code is indeed called FormationViolation (for whatever reason this was changed in v2). I believe this might just be the only change between the two ocppj versions. Fixing one version would break the other. To not break anything we probably need to inject error codes from the top layer. I would suggest splitting this into 2 separate PRs.

For FormatViolation, currently I have no idea, whether to just add FormatViolation type and keep both (FormationViolation and FormatViolation) maybe, but the response CallError to client will still and always use FormationViolation on your top layer ParseMessage method

ocpp-go/ocppj/ocppj.go

Lines 373 to 390 in c34f6ad

func (endpoint *Endpoint) ParseMessage(arr []interface{}, pendingRequestState ClientState) (Message, error) {
// Checking message fields
if len(arr) < 3 {
return nil, ocpp.NewError(FormationViolation, "Invalid message. Expected array length >= 3", "")
}
rawTypeId, ok := arr[0].(float64)
if !ok {
return nil, ocpp.NewError(FormationViolation, fmt.Sprintf("Invalid element %v at 0, expected message type (int)", arr[0]), "")
}
typeId := MessageType(rawTypeId)
uniqueId, ok := arr[1].(string)
if !ok {
return nil, ocpp.NewError(FormationViolation, fmt.Sprintf("Invalid element %v at 1, expected unique ID (string)", arr[1]), uniqueId)
}
// Parse message
if typeId == CALL {
if len(arr) != 4 {
return nil, ocpp.NewError(FormationViolation, "Invalid Call message. Expected array length 4", uniqueId)

@dwibudut
Copy link
Contributor Author

dwibudut commented Jul 4, 2023

New issue #210

@dwibudut dwibudut closed this Jul 4, 2023
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