Add helpers to set charge point common data#1026
Conversation
mhei
requested review from
Pietfried,
hikinggrass,
maaikez and
mlitre
as code owners
| void update_chargepoint_information(const std::string& chargepoint_vendor, const std::string& chargepoint_model, | ||
| const std::string& chargepoint_serialnumber, | ||
| const std::optional<std::string>& firmware_version); |
There was a problem hiding this comment.
nit: I would prefer vendor, model, serialnumber since `chargepoint is already part of the function name
| } | ||
| } | ||
|
|
||
| void ChargePointConfiguration::setChargepointInformation(const std::string& chargePointVendor, |
There was a problem hiding this comment.
Please add other properties of the BootNotification as well:
- iccid
- imsi
- meterSerialNumber
- meterType
- chargeBoxSerialNumber
There was a problem hiding this comment.
ok, will add as std::optional parameter
| this->config["Internal"]["ChargePointVendor"] = chargePointVendor; | ||
| this->config["Internal"]["ChargePointModel"] = chargePointModel; | ||
| this->config["Internal"]["ChargePointSerialNumber"] = chargePointSerialNumber; | ||
| if (firmwareVersion.has_value()) { | ||
| this->config["Internal"]["FirmwareVersion"] = firmwareVersion.value(); | ||
| } |
There was a problem hiding this comment.
In order to persist these changes they need to be written into the user_config.json. This can be done using the set_in_user_config function.
There was a problem hiding this comment.
I guess you refer to ChargePointConfiguration::setInUserConfig ? Hm, I'm unsure about this. When I call this function for each parameter, then this would result in multiple writes to the same file on disk. So if you insist that this should be made persistent, then I'd merge this functionality into a combined write.
But on the other hand: in my target use-case, it is not necessary to make these changes persistent since this function would be called during each boot and thus the data is always refreshed. Actually, the idea behind is that there is some common place outside the EVerest environment where such common charger data is stored permanently. Thus storing it again in the OCPP configuration file would not make sense, and especially not for e.g. IMSI or meter serial number (components which could be replaced during e.g maintenance...).
On the other hand, it would be written anyway as part of yet another key change...
There was a problem hiding this comment.
Yes, I was indeed refering to setInUserConfig.
I understand your use case, but libocpp itself is not aware of your target use case that calls the function on each boot, so I would like to treat the changes of these configuration keys like all the others. Otherwise this functionality should rather go into the start function or the ChargePoint constructor.
Of course you can merge the persisting functionality into a combined write. We are aware that the configuration handling for ocpp1.6 has some room for improvements in general.
|
Hi @mhei , are you planning to revisit this? Feel free to let us know if you need help with finishing this off |
|
Hi @Pietfried , I just need more free time 😄 Actually, I rebased my working setup to latest main branch yesterday evening and I plan to work on it this weekend. |
mhei
force-pushed
the
feature/charger_information_for_2025-03-28
branch
from
2b58c86 to
e87c465
Compare
mhei
force-pushed
the
feature/charger_information_for_2025-03-28
branch
from
e87c465 to
3005966
Compare
Pietfried
left a comment
Pietfried
left a comment
There was a problem hiding this comment.
Hi @mhei , thanks for picking this up again! I like your decision of spliting the update into three separate methods and the general structure of the PR looks good!
I would prefer that you use just std::optional instead of NullOrOptionalString because it is more in line with the existing interface and implementation. I know that this provides a little less flexibility because it doesnt allow the consumer to clear exsting keys, but the existing interface also doesnt allow this and afaics the companion PR in everest-core also can not make use of the clearing functionality anyways.
We are considering to refactor the whole configuration interface and implementation at some point and this is easier to do that if there aren't multiple approaches to updating config keys.
My suggestion is to:
- update the value if the optional is set
- keep the existing value if the optional is not set
The idea is to overwrite this data later, for example if other data sources are available than those available at the time of initialization. For this, 3 new methods are introduced which covers the three common data domains like general common data, modem and meter related data. Signed-off-by: Michael Heimpold <[email protected]>
|
Hi @Pietfried , I agree to your approach and reworked it as you suggested. Now I squashed all into a single commit, I think/hope it will make the review easier. Indeed, the companion PR only use a subset of this new functionality, but I expect there additional reviews and a possible rework, too. Let's see. |
mhei
force-pushed
the
feature/charger_information_for_2025-03-28
branch
from
3005966 to
e51ee78
Compare
Pietfried
left a comment
Pietfried
left a comment
There was a problem hiding this comment.
Thanks for applying the changes, lgtm!
The idea is to overwrite this data later, for example if other data sources are available than those available at the time of initialization. For this, 3 new methods are introduced which covers the three common data domains like general common data, modem and meter related data. Signed-off-by: Michael Heimpold <[email protected]>
4 participants
Describe your changes
The idea is to overwrite this data later, for example if other data sources are available than those available at the time of initialization. This PR adds the required helper methods for the OCPP 1.6 implementation.
Note: since I currently only have a OCPP 1.6 test environment at hand and my knowledge about OCPP 2.x is still limited, help with OCPP 2.x part would be welcome.
Issue ticket number and link
Checklist before requesting a review