Refactor/bsp interface #379

corneliusclaussen · 2023-10-09T21:01:43Z

No description provided.

lategoodbye · 2023-10-10T08:39:27Z

Hi @corneliusclaussen
thanks for sharing this pull request. Could you please elaborate the motivation of this pull request? Is it to move as much as possible IEC 61851 specific stuff out of the BSP?

corneliusclaussen · 2023-10-10T09:00:35Z

This is a refactoring that was planned for quite some time. As it breaks the BSP interfaces in an incompatible way, all BSP drivers will need to be changed. It should be relatively easy though.
Main motivations:

board_support_AC was used for DC as well for a while now but some things were missing for proper DC support. Also naming was wrong
To synchronize with the charging state machine in evsemanager abstract events were used (such as CarPluggedIn). This proved to be complex to use as the hardware typically knows states ABCDEF and the translation was non-trivial. To simplify BSP development, remove complexity from BSP drivers and avoid code duplication this code was moved into EvseManager. We decided to still use those abstract events internally in EvseManager as they hide some nasty details of IEC61851-1 (such as simplified charging mode). Now the BSP needs to report only the states ABCDEF. Most BSP drivers now only contain protocol translation code to the MCU and no logic anymore.
Factor out AC RCD. The RCD was in the board_support_AC interface before, in order to make the design a little cleaner it was moved to a separate interface (just like the IMD for DC charging)

In addition a lot of small improvements were made.

As this breaks code for many users we will also announce this on the mailing list as well as in todays community dev meeting

corneliusclaussen · 2023-10-10T09:27:53Z

Another note: EvseManager itself will need internal refactoring as well at some point as it got quite complex, but for now this is more about improving the interfaces to the outside so that we hopefully do not need to break BSP compatibility soon again.

corneliusclaussen · 2023-10-10T09:37:21Z

Please especially review the following:

interfaces/evse_board_support.yaml
types/board_support_common.yaml
types/evse_board_support.yaml

mooraby

General notes:

Not sure what guidelines do you follow, but what about using "this" to improve readability?
Code documentation

interfaces/evse_board_support.yaml

types/board_support_common.yaml

modules/EvseManager/BspStateMachine.cpp

modules/EvseManager/BspStateMachine.hpp

modules/EvseManager/BspStateMachine.cpp

types/board_support_common.yaml

types/evse_board_support.yaml

modules/EvseManager/BspStateMachine.cpp

types/board_support_common.yaml

interfaces/evse_board_support.yaml

mooraby · 2023-10-11T15:50:13Z

I will need to do another round of review, but at least I can confirm that the SIL is working now, also in multiple subsequent charging sessions.

lategoodbye · 2023-10-14T16:19:47Z

Hi,
on our website there is a detailed list (chapter 3.3.4 ff) of all error codes from our old charging stack. If i ignore all persistence related errors there are still some errors which are not available (e.g. lock capacitor not charged) above.

How should we handle these? Add them on demand later?

corneliusclaussen · 2023-10-16T07:25:30Z

About the BspEvent error codes: I'm trying to collect all useful error codes in this enum at the moment. I'll have a look at your list as well and will include them, I think it is a good moment as we are breaking things anyway right now. Of course also more can be added later on.
Before this PR gets undrafted, we want to also port it over to the new error handling framework, see PR #284. So the error list collected in the BspEvent enum is just a temporary list to collect all error codes that we want to be able to report.

corneliusclaussen · 2023-10-16T07:42:22Z

I looked through the ChargeControl C documentation and added the lock motor failure details. I renamed them a bit to align better with the wording used in MREC codes. The other error codes such as RCD and powermeter do not belong to this interface, they should go into the ac_rcd and powermeter interfaces. Those will become reportable errors as well soon with the new error handling framework.

modules/EvseManager/IECStateMachine.cpp

interfaces/evse_board_support.yaml

types/evse_manager.yaml

types/board_support_common.yaml

lategoodbye · 2023-10-19T12:16:03Z

@corneliusclaussen I have a question regarding the detection of an EV. It's possible to implement this via different approaches (only CP, only PP, mixed CP/PP).

Is it up to the implementor of the board support package to decide if an EV is connected or is there some kind of EVerest policy how to detected this?

mhei · 2023-10-19T12:39:17Z

@corneliusclaussen: Would it be an option for you to cut the plug lock stuff out into a dedicated interface? This would make the bsp interface even more generic and minimalistic. BSPs with plug lock capabilities could easily implement both interfaces in a single module (e.g. Yeti if I'm right) and boards with no hardware support at all could easily just skip it. On platform like our Tarragon, we have for example two hardware plug lock interfaces. Our plan would be to create a module which cares about a single one, but customers could use another module instance for the second plug lock (e.g. for locking a fixed cable while a socket is in use...).

corneliusclaussen · 2023-10-19T12:41:57Z

@corneliusclaussen I have a question regarding the detection of an EV. It's possible to implement this via different approaches (only CP, only PP, mixed CP/PP).

Is it up to the implementor of the board support package to decide if an EV is connected or is there some kind of EVerest policy how to detected this?

No, the BSP just sends the states etc, it does not need to find out if a car is connected or not. The norm leaves that as a choice for the implementation afaik.

EvseManager always uses CP to detect a plugin. If configured for Socket (non tethered) it will use PP afterwards to determine the current limit (and set it to 0 if PP value is invalid).

This means: attached cable: CP only (nothing else is possible)
Socket: CP+PP needs to be correct

I do not see an application where PP only makes sense?

corneliusclaussen · 2023-10-19T12:48:35Z

@corneliusclaussen: Would it be an option for you to cut the plug lock stuff out into a dedicated interface? This would make the bsp interface even more generic and minimalistic. BSPs with plug lock capabilities could easily implement both interfaces in a single module (e.g. Yeti if I'm right) and boards with no hardware support at all could easily just skip it. On platform like our Tarragon, we have for example two hardware plug lock interfaces. Our plan would be to create a module which cares about a single one, but customers could use another module instance for the second plug lock (e.g. for locking a fixed cable while a socket is in use...).

In this case EvseManager would need to decide when to lock/unlock. In all HW that we have here this is actually done by an MCU (which ensures proper unlocking in case of Linux fails etc). It could be done though as the MCU can still do it on its own and not use the evsemanager supplied information. I factored out the RCD for the same reason of . I can make a draft, but probably after charin testival

corneliusclaussen · 2023-10-20T06:58:16Z

@corneliusclaussen: Would it be an option for you to cut the plug lock stuff out into a dedicated interface? This would make the bsp interface even more generic and minimalistic. BSPs with plug lock capabilities could easily implement both interfaces in a single module (e.g. Yeti if I'm right) and boards with no hardware support at all could easily just skip it. On platform like our Tarragon, we have for example two hardware plug lock interfaces. Our plan would be to create a module which cares about a single one, but customers could use another module instance for the second plug lock (e.g. for locking a fixed cable while a socket is in use...).

@mhei : Another though on this: If we refactor out all motor lock things into a separate interface, would it make sense to also move all PP related things to this motor lock interface? If there is a socket it also requires a lock and only with a socket PP is useful?

If there is a fixed attached cable you don't need motor lock but you also don't need PP. What do you think?

interfaces/evse_board_support.yaml

mhei · 2023-10-20T08:17:57Z

In this case EvseManager would need to decide when to lock/unlock. In all HW that we have here this is actually done
by an MCU (which ensures proper unlocking in case of Linux fails etc). It could be done though as the MCU can still do it
on its own and not use the evsemanager supplied information. I factored out the RCD for the same reason of .

I like this idea. Reason is that when we factor out RCD stuff, then IMHO the EvseManager is in charge to control whether the plug should be pull-able from the socket or not and not the BSP (since it does not have the information about the tripped RCD/RCM). (I such an error case, this could then e.g. require explicit force-removal trigger via OCPP...)

@mhei : Another though on this: If we refactor out all motor lock things into a separate interface, would it make sense to
also move all PP related things to this motor lock interface? If there is a socket it also requires a lock and only with
a socket PP is useful?
If there is a fixed attached cable you don't need motor lock but you also don't need PP. What do you think?

I agree that this is usually related, but as I mentioned, I'd like to (re-)use the lock motor interface for other purposes as well. Having then a stray PP interface in this interface would make things more complex on this side.

corneliusclaussen · 2023-10-20T08:31:20Z

I agree that this is usually related, but as I mentioned, I'd like to (re-)use the lock motor interface for other purposes as well. Having then a stray PP interface in this interface would make things more complex on this side.

Agreed. Then we leave PP in the bsp interface and create a new one with just the lock/unlock commands and error reports for the lock

corneliusclaussen · 2023-10-20T09:45:33Z

I agree that this is usually related, but as I mentioned, I'd like to (re-)use the lock motor interface for other purposes as well. Having then a stray PP interface in this interface would make things more complex on this side.

Agreed. Then we leave PP in the bsp interface and create a new one with just the lock/unlock commands and error reports for the lock

I pushed a draft for the interfaces/connector_lock.yaml, pls have a look. It is not used that, I'll implement in evsemanager once the interface is agreed

mhei · 2023-10-20T11:24:39Z

@corneliusclaussen : connector lock interface (aka motor lock interface) LGTM, thanks for addressing this topic so quickly 👍

FaHaGit · 2023-11-09T12:43:56Z

Hello together,

We at chargebyte would like to give you a brief update on the current draft of the BSP interface. We have tested the new interface for CP/PP and contactor handling with our hardware platform for the AC path. Everything seems to work as expected with the EvseManager. Some comments are still open from the code review, but once these are answered we see no problems to create a pull request from it.

Also the reported bug with resuming of the charging session by the EV (CP B -> C -> B -> C transition) works now after we updated everest framework.

We know that it will probably take a lot of effort on your side to prepare everything that it can be merged into erverest-core, but we are looking forward to working with the new interface soon :-).

Are there already plans when the new interface will be available?

Thank you and best regards

FaHaGit · 2023-11-09T15:44:39Z

One additional note to the CP B -> C -> B -> C transition scenario. There is still 100% duty cycle for a short time after the second switch from B to C. I think this not expected.

corneliusclaussen · 2023-11-10T10:58:16Z

Will look into the comments. Apart from that I would like to move all error logic over to the new error handling in framework. After that I will undraft the PR

modules/EvseManager/IECStateMachine.cpp

corneliusclaussen · 2023-12-20T16:09:06Z

All comments should be addressed now. It was tested both in SIL as well as on real hardware.

- The new evse_board_support interface replaces the old board_support_AC interface both for AC and DC. - Move from abstract events (e.g. CarPluggedIn) to CP states A/B/C/D/E/F to simplify BSP drivers - This moves the complete IEC61851-1 state machine into EvseManager - create new interfaces for AC_RCD and connector_lock - Charging subsystem is now using the new error framework - Added most MREC error types that relate to charging - Yeti and umwc driver will need MCU firmware updates that will come soon - Added most MREC fault codes to NodeRed SIL UI Signed-off-by: Cornelius Claussen <[email protected]>

modules/EvseManager/Charger.cpp

modules/EvseManager/Charger.hpp

Add {} to if Co-authored-by: Kai Hermann <[email protected]> Signed-off-by: corneliusclaussen <[email protected]>

Signed-off-by: Cornelius Claussen <[email protected]>

hikinggrass · 2023-12-21T14:29:52Z

modules/YetiDriver/board_support/evse_board_supportImpl.cpp

+            if (caps_received)
+                break;


Suggested change

if (caps_received)

break;

if (caps_received) {

break;

}

corneliusclaussen force-pushed the refactor/bsp_interface branch 2 times, most recently from 243fe8d to 1072f72 Compare October 9, 2023 21:05

mooraby reviewed Oct 10, 2023

View reviewed changes

corneliusclaussen force-pushed the refactor/bsp_interface branch from e396e87 to 3420b10 Compare October 11, 2023 14:33

FaHaGit reviewed Oct 11, 2023

View reviewed changes

types/board_support_common.yaml Show resolved Hide resolved

types/board_support_common.yaml Outdated Show resolved Hide resolved

interfaces/evse_board_support.yaml Show resolved Hide resolved

corneliusclaussen mentioned this pull request Oct 16, 2023

Questions regarding BCD events in CP state machine and state D in CP state machine. PionixPublic/yeti-firmware#3

Closed

corneliusclaussen force-pushed the refactor/bsp_interface branch from 13b76d2 to 3bdd847 Compare October 16, 2023 07:43

mooraby reviewed Oct 17, 2023

View reviewed changes

modules/EvseManager/IECStateMachine.cpp Show resolved Hide resolved

modules/EvseManager/IECStateMachine.cpp Outdated Show resolved Hide resolved

modules/EvseManager/IECStateMachine.cpp Outdated Show resolved Hide resolved

mooraby reviewed Oct 19, 2023

View reviewed changes

interfaces/evse_board_support.yaml Show resolved Hide resolved

mooraby reviewed Oct 19, 2023

View reviewed changes

types/evse_manager.yaml Outdated Show resolved Hide resolved

types/board_support_common.yaml Outdated Show resolved Hide resolved

corneliusclaussen force-pushed the refactor/bsp_interface branch from 5c95274 to 0889146 Compare October 20, 2023 06:59

mooraby reviewed Oct 20, 2023

View reviewed changes

interfaces/evse_board_support.yaml Outdated Show resolved Hide resolved

corneliusclaussen force-pushed the refactor/bsp_interface branch from c6b55b5 to 5f8780e Compare October 20, 2023 09:27

mooraby reviewed Nov 14, 2023

View reviewed changes

modules/EvseManager/IECStateMachine.cpp Outdated Show resolved Hide resolved

modules/EvseManager/IECStateMachine.cpp Show resolved Hide resolved

corneliusclaussen force-pushed the refactor/bsp_interface branch 2 times, most recently from b2f68eb to c2e57a7 Compare November 22, 2023 16:27

drmrd mentioned this pull request Dec 6, 2023

Impose resource constraints on demos EVerest/everest-demo#6

Merged

corneliusclaussen force-pushed the refactor/bsp_interface branch 4 times, most recently from 853ef93 to dea0de0 Compare December 13, 2023 12:58

corneliusclaussen force-pushed the refactor/bsp_interface branch 2 times, most recently from 5d48b56 to 0eb2a50 Compare December 20, 2023 16:08

corneliusclaussen force-pushed the refactor/bsp_interface branch from fec80d9 to d8a6680 Compare December 20, 2023 16:13

corneliusclaussen marked this pull request as ready for review December 20, 2023 16:14

corneliusclaussen force-pushed the refactor/bsp_interface branch 3 times, most recently from bded59b to 0c61816 Compare December 20, 2023 16:51

corneliusclaussen force-pushed the refactor/bsp_interface branch from 0c61816 to 02382f7 Compare December 21, 2023 09:39