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

Added charging schedules definition #582

Merged
merged 9 commits into from
Apr 10, 2024

Conversation

james-ctc
Copy link
Contributor

@james-ctc james-ctc commented Mar 13, 2024

Describe your changes

Create YAML/JSON definition for charging schedules.

Note this is an API breaking change. Originally charging schedules were published:

    { "0": {...}, "1": {...}}

Using the connector number as the key. This is not supported in the YAML interface or type definitions.

Instead an array is used with an additional key added:

    [ { "connector": 0, ...}, { "connector": 1, ...} ... ]

Unfortunately the generated code doesn't like arrays at that point, so the array has been wrapped in an object:

    { "schedules": [ { "connector": 0, ...}, { "connector": 1, ...} ... ] }

Note is should not be assumed that the array index will match the connector number.

The existing types in libocpp have not been changed. A new mapping has been created in the OCPP module to convert from the internal format to the MQTT JSON format.

Note OCPP201 doesn't publish schedules at the moment and has not changed.

Issue ticket number and link

None

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

Note this is an API breaking change. Originally charging schedules were
published:

{ "0": {...}, "1": {...}}

using the connector number as the key. This is not supported
in the YAML interface or type definitions.
Instead an array is used with an additional key added:

[ { "connector": 0, ...}, { "connector": 1, ...} ... ]

Note is should not be assumed that the array index will match the connector
number.

The existing types in libocpp have not been changed. A new mapping
has been created in the OCPP module.
(OCPP201 doesn't publish schedules at the moment)

Signed-off-by: James Chapman <[email protected]>
The autogenerated code doesn't support a module var being an array
(of a specific object type)

[{},{}]

Instead the array is now wrapped in an object:

{"schedules": [{},{}]}

Signed-off-by: James Chapman <[email protected]>
@james-ctc james-ctc marked this pull request as ready for review March 13, 2024 16:08
@james-ctc james-ctc removed the request for review from corneliusclaussen March 13, 2024 16:09
@james-ctc james-ctc self-assigned this Mar 13, 2024
Copy link
Contributor

@Pietfried Pietfried left a comment

Choose a reason for hiding this comment

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

Types and interfaces look good from my side.

Although the definitions in the OCPP spec are camelCase, we try to stick to snake_case in our internal interfaces. I have added some suggestions inline, but the code also needs to apply the changes.

@corneliusclaussen : Do we need changes on the EnergyManager side if we make these changes?

types/ocpp.yaml Outdated
Comment on lines 10 to 12
- startPeriod
- limit
- stackLevel
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- startPeriod
- limit
- stackLevel
- start_period
- limit
- stack_level

types/ocpp.yaml Outdated
Comment on lines 14 to 21
startPeriod:
type: integer
limit:
type: number
numberPhases:
type: integer
stackLevel:
type: integer
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
startPeriod:
type: integer
limit:
type: number
numberPhases:
type: integer
stackLevel:
type: integer
start_period:
type: integer
limit:
type: number
number_phases:
type: integer
stack_level:
type: integer

types/ocpp.yaml Outdated
Comment on lines 28 to 29
- chargingRateUnit
- chargingSchedulePeriod
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- chargingRateUnit
- chargingSchedulePeriod
- charging_rate_unit
- charging_schedule_period

types/ocpp.yaml Outdated
connector:
type: integer
minimum: 0
chargingRateUnit:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
chargingRateUnit:
charging_rate_unit:

types/ocpp.yaml Outdated
minimum: 0
chargingRateUnit:
type: string
chargingSchedulePeriod:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
chargingSchedulePeriod:
charging_schedule_period:

types/ocpp.yaml Outdated
$ref: /ocpp#/ChargingSchedulePeriod
duration:
type: integer
startSchedule:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
startSchedule:
start_schedule:

types/ocpp.yaml Outdated
type: integer
startSchedule:
type: string
minChargingRate:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
minChargingRate:
min_charging_rate:

@@ -141,14 +142,46 @@ void OCPP::set_external_limits(const std::map<int32_t, ocpp::v16::EnhancedChargi
}
}

namespace {
types::ocpp::ChargingSchedulePeriod to_ChargingSchedulePeriod(const ocpp::v16::EnhancedChargingSchedulePeriod& period) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the conversions.hpp / cpp for these conversions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved functions to conversions.[ch]pp

fix: moved conversions to separate file

Signed-off-by: James Chapman <[email protected]>
@james-ctc
Copy link
Contributor Author

Although the definitions in the OCPP spec are camelCase, we try to stick to snake_case in our internal interfaces

The case naming came from struct EnhancedChargingSchedule rather than OCPP.
CamelCase is used quite a bit within ocpp.yaml e.g.:

  DataTransferStatus:
    description: Data Transfer Status enum
    type: string
    enum:
      - Accepted
      - Rejected
      - UnknownMessageId
      - UnknownVendorId

Updated required: and properties: sections to use snake_case
Left ChargingSchedules ChargingSchedule and ChargingSchedulePeriod CamelCase to match existing entries.

@james-ctc
Copy link
Contributor Author

james-ctc commented Mar 14, 2024

Do we need changes on the EnergyManager side if we make these changes?

It doesn't look like EnergyManager depends on the ocpp interface and I can't locate any MQTT subscribe for charging_schedules.

Looking at recent updates to main it seems the API module now subscribes for charging_schedules.

@james-ctc james-ctc force-pushed the feat/charging-schedule-definition branch from c310bf6 to fa3f464 Compare March 14, 2024 16:05
@Pietfried Pietfried assigned Pietfried and unassigned james-ctc Apr 2, 2024
@Pietfried Pietfried merged commit b135d38 into main Apr 10, 2024
5 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.

3 participants