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/376 ocpp 201 california pricing requirements tariff and cost #768

Conversation

maaikez
Copy link
Contributor

@maaikez maaikez commented Jul 8, 2024

Describe your changes

Interface changes needed for California Pricing Requirements: TariffAndCost and DisplayMessage.

Issue ticket number and link

libocpp / 376

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

maaikez added 4 commits July 4, 2024 17:19
Signed-off-by: Maaike Zijderveld, iolar <[email protected]>
Signed-off-by: Maaike Zijderveld, iolar <[email protected]>
…se it for other than ocpp messages as well.

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>
@maaikez maaikez assigned maaikez and unassigned maaikez Jul 8, 2024
maaikez added 4 commits July 8, 2024 16:58
Signed-off-by: Maaike Zijderveld, iolar <[email protected]>
…e this for other things than only ocpp as well probably.

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>
message_format:
description: The format of the message
type: string
$ref: /ocpp#/MessageFormat
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a reference to an ocpp type, which we should try to avoid

Copy link
Contributor

Choose a reason for hiding this comment

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

Move over to display_message

Comment on lines 17 to 27
properties:
status:
description: Whether the charging station is able to display the message
$ref: /display_message#/DisplayMessageStatusEnum
type: object
status_info:
description: Detailed status information
$ref: /ocpp#/StatusInfoType
type: object
required:
- status
Copy link
Contributor

Choose a reason for hiding this comment

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

This has to be defined in another type definition like SetDisplayMessageResponse

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will, but it does just build.

properties:
status_info:
description: Detailed status information
$ref: /ocpp#/StatusInfoType
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a reference to an ocpp type, which we should try to avoid

type: object
additionalProperties: false
properties:
graceMinutes:
Copy link
Contributor

Choose a reason for hiding this comment

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

we prefer to use snake_case for properties

Grace minutes start counting from the timestamp of the message in which state
changed from "Charging" to "Idle".
type: integer
hourPrice:
Copy link
Contributor

Choose a reason for hiding this comment

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

we prefer to use snake_case for properties

@@ -0,0 +1,83 @@
description: >-
This interface allows to add / remove / change messages that should be showed on the display.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a more detailed description what to expect from a module that implements this interface e.g.:

  • store and retrieve predefined messages
  • Show messages on a display
  • ...

cost:
description: >-
Cost of the energy (or other things like time, base price, ...)
consumed during this chunk in the currency specified in the session
price
type: object
$ref: /money#/MoneyAmount
costAmount:
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be dropped

description: >-
Price per hour while idle
type: number
ChargingPriceComponent:
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept it but changed the enum type to 'category'. Because we need an array with this information in SessionCost (otherwise I have to define it there, which is also fine by me).

Copy link
Contributor

Choose a reason for hiding this comment

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

but what is the difference to the SessionCostChunk?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A SessionCostChunk is the 'real cost' of the session (the total cost of the charging session until now for example), ChargingPriceComponent is the price per kWh, hour or flat fee. So if you want to calculate the total cost (for example if the charging station goes offline), you need the charging price components to be able to do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

understood - but can it be modeled similarly to the SessionCostChunk and also named similarly, e.x. PriceChunk and allow the user to pass an array of chunks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well that is only renaming ChargingPriceComponent to PriceChunk isn't it? As it already is an array.

SessionStatus:
description: >-
Session status enum. Session can be running or finished. Costs of the
running session are not final and can change.
type: string
enum:
- Running
- Idle
- Finished
SessionCost:
Copy link
Contributor

Choose a reason for hiding this comment

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

id_tag should move to optional and session_id to required to identify the session

description: Whether the charging station is able to display the message
$ref: /display_message#/DisplayMessageStatusEnum
type: object
status_info:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be removed from this type definition / make it a string

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>
@maaikez maaikez force-pushed the feature/376-ocpp-201-california-pricing-requirements-tariff-and-cost branch from e6548b6 to dbb9033 Compare July 10, 2024 14:26
@@ -1,5 +1,62 @@
description: Types to provide the session price
types:
IdlePrice:
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm a little confused - from what i understand we refer to idle to the state when the station has no customer. What does idle mean in this context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For more info, you can read the California Pricing Requirements.
https://openchargealliance.org/whitepapers/ <- to be downloaded there.

Idle is when a car is attached but currently not charging. For example because the battery is already full. There are charging stations where you have to pay something when you are not charging (I mainly found them in the field with DC chargers, but also AC chargers sometimes have this).

Copy link
Contributor

Choose a reason for hiding this comment

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

cool understood - maybe you can link the req in the description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes the link is already in the issue:
EVerest/libocpp#376

type: object
properties:
timestamp_from:
description: Time when these prices become active.
Copy link
Contributor

Choose a reason for hiding this comment

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

should be singular as in other comments below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it refers to the charging prices and idle prices.

- Energy
- Time
- Other
$ref: /session_cost#/CostCategory
SessionStatus:
description: >-
Session status enum. Session can be running or finished. Costs of the
Copy link
Contributor

Choose a reason for hiding this comment

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

the doc is then outdated

metervalue_from:
description: >-
Metervalue (energie Wh import) for the start of this chunk
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.

why not using powermeter types here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this is as specified in California Pricing Requirements. We just get a metervalue from the CSMS. But if you think we should change this: what is your proposal?

Copy link
Contributor

Choose a reason for hiding this comment

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

i would use the powermeter type since it would fit better in the ecosystem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i would use the powermeter type since it would fit better in the ecosystem

I am fine with that, @Pietfried is that ok for you?

Copy link
Contributor

Choose a reason for hiding this comment

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

The chunk is really only defined for a range energy_wh_import right? In that case the powermeter type could bring in a lot of irrelevant data (voltage, reactive power) and even lead to confusion what this property actually means within this type, so I would stick with the current definition

description: Cost category
type: string
enum:
- Energy
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we model the idle price not as a category in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you would like to add a IdlePriceTime, ChargingTime, IdleGraceMinutes and remove Time? I thought about it but I don't know if that makes things more clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

i don't really understand why we need to define actually idle as a separate category. Your linked whitepater says

It is up to the CSMS and not the charging station, to decide when a transaction is considered to be in idle time.

So at some point when the station sends a chargingState or atPowerkW and the CSMS thinks that its now idle it will send a price containing just the time component. The CSMS can internally also account for grace times or other conditions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes but what if the CSMS is not online?
I just reflect the fields from the California Pricing Requirements, which sends the idle price as well.

maaikez added 2 commits July 11, 2024 12:35
Signed-off-by: Maaike Zijderveld, iolar <[email protected]>
…ChargingPriceComponent instead of Money because it includes the number of decimals. Add more conversions.

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>
@maaikez maaikez force-pushed the feature/376-ocpp-201-california-pricing-requirements-tariff-and-cost branch 2 times, most recently from 56d716f to fce06f8 Compare July 12, 2024 15:54
… and session cost)

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>
@maaikez maaikez force-pushed the feature/376-ocpp-201-california-pricing-requirements-tariff-and-cost branch from fce06f8 to 00fd07c Compare July 12, 2024 15:58
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.

Looks good overall, see in line comments 👍

types/display_message.yaml Outdated Show resolved Hide resolved
types/display_message.yaml Show resolved Hide resolved
types/display_message.yaml Outdated Show resolved Hide resolved
types/display_message.yaml Outdated Show resolved Hide resolved
types/display_message.yaml Show resolved Hide resolved
modules/OCPP/conversions.hpp Outdated Show resolved Hide resolved
modules/OCPP/conversions.hpp Outdated Show resolved Hide resolved
types/session_cost.yaml Outdated Show resolved Hide resolved
if (timestamp.has_value()) {
chunk.timestamp_to = timestamp.value().to_rfc3339();
}
chunk.metervalue_to = meter_value;
Copy link
Contributor

Choose a reason for hiding this comment

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

What about metervalue_from ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well in ocpp there is no metervalue_from. But there is a metervalue_from and metervalue_to in the interface (they were already there).

metervalue_from:
description: >-
Metervalue (energie Wh import) for the start of this chunk
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.

The chunk is really only defined for a range energy_wh_import right? In that case the powermeter type could bring in a lot of irrelevant data (voltage, reactive power) and even lead to confusion what this property actually means within this type, so I would stick with the current definition

maaikez added 7 commits July 30, 2024 13:34
Signed-off-by: Maaike Zijderveld, iolar <[email protected]>
Signed-off-by: Maaike Zijderveld, iolar <[email protected]>
…pp display message struct as well.

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>
…nstead of a number (double)

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>
…it is not only about display messages.

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>
@@ -60,7 +60,7 @@ libevse-security:
# OCPP
libocpp:
git: https://github.com/EVerest/libocpp.git
git_tag: aab1d5785bde141203b1a89b03d66fa91edf8093
git_tag: 1009ca1550a54824b9e2c1eacdad69f9373fd542
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO set to correct git tag before merging to main!!!

maaikez added 2 commits August 5, 2024 16:51
Signed-off-by: Maaike Zijderveld, iolar <[email protected]>
Signed-off-by: Maaike Zijderveld, iolar <[email protected]>
@maaikez maaikez marked this pull request as ready for review August 5, 2024 15:36
maaikez and others added 2 commits August 6, 2024 10:40
@@ -0,0 +1,31 @@
// SPDX-License-Identifier: Apache-2.0
Copy link
Contributor

Choose a reason for hiding this comment

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

the whole display_message directory shall be removed from OCPP. Its not something that the module provides, but optionally requires (see the manifest)

const types::session_cost::SessionCost cost =
conversions::create_session_cost(session_cost, number_of_decimals, {});
ocpp::v16::DataTransferResponse response;
if (this->p_session_cost == nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: p_session_cost cannot really be null, since providing an interface implementation is not optional for a module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok i'll remove it. on your risk ;)

@maaikez maaikez merged commit da0da54 into main Aug 13, 2024
11 checks passed
@maaikez maaikez deleted the feature/376-ocpp-201-california-pricing-requirements-tariff-and-cost branch August 13, 2024 15:39
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.

OCPP 2.0.1 California Pricing Requirements: Tariff and Cost
5 participants