Introduction of CbCPXDriver for upcoming new product "Charge Point eXtender"#47
Introduction of CbCPXDriver for upcoming new product "Charge Point eXtender"#47
Conversation
barsnick
left a comment
barsnick
left a comment
There was a problem hiding this comment.
Great job, and what a big effort!
I don't think any of my comments are functional. Just one big compile problem with the dependency of CbCpx to CbCpxDriver.
The rest is cosmetic.
After fixing everything, be sure to run clang-format -i to fix indentation, header order, and stuff.
config/cpx-can-test-sil.yaml
Outdated
There was a problem hiding this comment.
I think if you name this file config/config-cpx-can-sil.yaml, and add generate_config_run_script(CONFIG cpx-can-sil) to config/CMakeLists.txt, it will create a run script for you. Or perhaps not in external repositories - not quite sure. 😉
config/cpx-can-test-sil.yaml
Outdated
| active_modules: | ||
| api: | ||
| connections: | ||
| evse_manager: | ||
| - implementation_id: evse | ||
| module_id: connector_1 | ||
| error_history: | ||
| - module_id: error_history | ||
| implementation_id: error_history | ||
| module: API |
There was a problem hiding this comment.
For readability, we try to keep the module: entries at the top, i.e.:
| active_modules: | |
| api: | |
| connections: | |
| evse_manager: | |
| - implementation_id: evse | |
| module_id: connector_1 | |
| error_history: | |
| - module_id: error_history | |
| implementation_id: error_history | |
| module: API | |
| active_modules: | |
| api: | |
| module: API | |
| connections: | |
| evse_manager: | |
| - implementation_id: evse | |
| module_id: connector_1 | |
| error_history: | |
| - module_id: error_history | |
| implementation_id: error_history |
(Also for the other entries below.)
config/cpx-can-test-sil.yaml
Outdated
| ac_hlc_enabled: true | ||
| ac_hlc_use_5percent: false | ||
| ac_nominal_voltage: 230 | ||
| charge_mode: AC |
There was a problem hiding this comment.
Isn't our major use case DC?
config/cpx-can-test-sil.yaml
Outdated
| config_module: | ||
| device: auto | ||
| supported_ISO15118_2: true | ||
| connections: {} |
There was a problem hiding this comment.
An empty connections object is meaningless and can be omitted. (Yes it's also used in the upstream configs.)
| connections: {} |
Also in the other modules in this config.
| if (state_change) | ||
| EVLOG_info << "handle_allow_power_on: request to " << (value.allow_power_on ? "CLOSE" : "OPEN") | ||
| << " the contactor"; | ||
| else | ||
| EVLOG_debug << "handle_allow_power_on: request to " << (value.allow_power_on ? "CLOSE" : "OPEN") | ||
| << " the contactor"; |
There was a problem hiding this comment.
Try not to create the same string twice.
| if (state_change) | |
| EVLOG_info << "handle_allow_power_on: request to " << (value.allow_power_on ? "CLOSE" : "OPEN") | |
| << " the contactor"; | |
| else | |
| EVLOG_debug << "handle_allow_power_on: request to " << (value.allow_power_on ? "CLOSE" : "OPEN") | |
| << " the contactor"; | |
| std::ostringstream ss; | |
| ss << "handle_allow_power_on: request to " << (value.allow_power_on ? "CLOSE" : "OPEN") | |
| << " the contactor"; | |
| if (state_change) | |
| EVLOG_info << ss.str(); | |
| else | |
| EVLOG_debug << ss.str(); |
| return; | ||
| } | ||
|
|
||
| int code_switch_state = this->mod->controller->switch_state(value.allow_power_on); |
There was a problem hiding this comment.
const
| int code_switch_state = this->mod->controller->switch_state(value.allow_power_on); | |
| const int code_switch_state = this->mod->controller->switch_state(value.allow_power_on); |
| EVLOG_info << "Current state: " << (this->mod->controller->get_contactor_state() ? "CLOSED" : "OPEN"); | ||
|
|
||
| // publish PowerOn or PowerOff event | ||
| types::board_support_common::Event tmp_event = value.allow_power_on |
There was a problem hiding this comment.
const
| types::board_support_common::Event tmp_event = value.allow_power_on | |
| const types::board_support_common::Event tmp_event = value.allow_power_on |
| types::board_support_common::BspEvent tmp {tmp_event}; | ||
| this->publish_event(tmp); | ||
| } else { | ||
| types::cb_board_support::ContactorState actual_state = !(value.allow_power_on) |
There was a problem hiding this comment.
const
| types::cb_board_support::ContactorState actual_state = !(value.allow_power_on) | |
| const types::cb_board_support::ContactorState actual_state = !(value.allow_power_on) |
| } // namespace temperatures | ||
| } // namespace module | ||
|
|
||
| #endif // TEMPERATURES_CB_CHARGESOM_TEMPERATURES_IMPL_HPP |
There was a problem hiding this comment.
Incorrect comment. This will fix itself if you re-run ev-cli.
| #endif // TEMPERATURES_CB_CHARGESOM_TEMPERATURES_IMPL_HPP | |
| #endif // TEMPERATURES_CB_CPX_TEMPERATURES_IMPL_HPP |
| this->cp_current_state = types::cb_board_support::CPState::PilotFault; | ||
| } | ||
| }); | ||
|
|
||
| this->mod->controller->on_contactor_error.connect( | ||
| [&](const std::string& source, bool desired_state, types::cb_board_support::ContactorState actual_state) { | ||
| [this](const std::string& source, bool desired_state, types::cb_board_support::ContactorState actual_state) { |
There was a problem hiding this comment.
Why did you use two different types for desired_state and actual_state?
There was a problem hiding this comment.
desired_state is used as bool because the contactors can either be open or closed. I thought it would be enough to use this binary type instead of enum which I use to make the semantics clearer for the log output.
There was a problem hiding this comment.
We have similar constructs in the other drivers; let's leave it as is for now and rework it later for all platforms.
modules/CbCpxDriver/cpx/CbCpx.cpp
Outdated
| } | ||
|
|
||
| // we should see the changes take effect after 1s (FIXME) | ||
| std::this_thread::sleep_for(std::chrono::seconds(1)); |
There was a problem hiding this comment.
What does “fixme” stand for? I would also appreciate not having to wait here for a fixed amount of time, especially because, as I see it, handle_power_on is blocked (mqtt receive thread).
barsnick
left a comment
barsnick
left a comment
There was a problem hiding this comment.
Thanks for the changes! Looking good. I only found some (further) minor stuff.
Also, as a last step, maybe still worth running clang-formatover it.
| Pal::Sigslot | ||
| nlohmann_json::nlohmann_json | ||
| everest::log | ||
| everest::sqlite |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
modules/CbCPXDriver/cpx/CbCPX.hpp
Outdated
| /// @brief Default constructor. | ||
| /// @param device_id Offset in CAN-ID used to identify hardware plattform | ||
| /// @param can_interface Name of the CAN-interface used for communication | ||
| CbCPX(int device_id, std::string can_interface, int can_bitrate); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| bool is_emergency(); | ||
|
|
||
| /// @brief Set a new duty cycle. | ||
| /// @param duty_cycle The desired duty cycle in percent [0.1 %]. |
There was a problem hiding this comment.
yes, this should be correct and is in sync with other safety chip based products
modules/CbCPXDriver/cpx/CbCPX.hpp
Outdated
| void set_duty_cycle(unsigned int duty_cycle); | ||
|
|
||
| /// @brief Set contactor state. | ||
| /// @param on Desired contactor state. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| /// @param data Received CAN data | ||
| void read_git_hash(uint8_t* data); | ||
|
|
||
| /// @brief Return the current contactor state (even when no contactor is configured) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
modules/CbCPXDriver/cpx/CbCPX.cpp
Outdated
| int bytes_sent = write(this->can_raw_fd, &frame, static_cast<size_t>(sizeof(frame))); | ||
| if (bytes_sent != static_cast<int>(sizeof(frame))) { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
modules/CbCPXDriver/cpx/CbCPX.cpp
Outdated
| gh_lock.unlock(); | ||
|
|
||
| bytes_sent = write(this->can_raw_fd, &frame, static_cast<size_t>(sizeof(frame))); | ||
| if (bytes_sent != static_cast<int>(sizeof(frame))) { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
mhei
changed the title
modules/CbCPXDriver/manifest.yaml
Outdated
| default: 80 | ||
| can_interface: | ||
| description: >- | ||
| Device name of the can interface to use for communication with CPX |
There was a problem hiding this comment.
I think we should use throughout the spelling "CAN" (all upper-case) since this is a well-known abbreviation.
There was a problem hiding this comment.
I have changed the naming throughout except for included and generated libraries.
modules/CbCPXDriver/manifest.yaml
Outdated
| default: can0 | ||
| can_bitrate: | ||
| description: >- | ||
| Bitrate of CAN bus used by CPX can controller. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| description: >- | ||
| Bitrate of CAN bus used by CPX can controller. | ||
| type: integer | ||
| default: 500000 |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
modules/CbCPXDriver/manifest.yaml
Outdated
| description: Enables or disables monitoring of an external Residual Current Device. | ||
| type: boolean | ||
| default: false | ||
| device_id: |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
modules/CbCPXDriver/manifest.yaml
Outdated
| description: Identification string used when this temperature channel is published | ||
| type: string | ||
| default: "PT1000-4" | ||
| rcm_enable: |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This adds support for CPX hardware platform. Signed-off-by: Erik Herrmann <erik.herrmann@chargebyte.com>
Signed-off-by: Erik Herrmann <erik.herrmann@chargebyte.com>
…lean up config, fix bug in reacting to PP changes - the id parameter was renamed to device_id to make it more intuitive - now the default can-interface is can0 - before a complete config-object was handed down from CbCpxDriver to CbCpx which is now done by handing over only the necessary values which are then saved into a config-object of CbCpx - a bug did not allow publishing the first PP-state change which is now fixed Signed-off-by: Erik Herrmann <erik.herrmann@chargebyte.com>
- remove unnecessary while-loop from CbCpxDriver - improve comment meaningfulness - use 1s instead of std::chrono::seconds(1) - remove double string-formatting - remove multiple identical function calls within same scope - remove unnecessary breaking condition - correct bug where contactor state was displayed wrong in log Output Signed-off-by: Erik Herrmann <erik.herrmann@chargebyte.com>
- qualify member access with this-> and use const locals - add safe casts for CAN frame sizes/IDs - fix lambda captures and minor logic/style cleanups - allow default args for temperature/contactor helpers Signed-off-by: Erik Herrmann <erik.herrmann@chargebyte.com>
- replace notify flag struct with bitset + helpers - simplify contactor/estop/pt1000 accessors with switch - remove unused includes and stale comments - drop unused Boost deps from cpx CMakeLists - update manifest wording to CPX platform Signed-off-by: Erik Herrmann <erik.herrmann@chargebyte.com>
Signed-off-by: Erik Herrmann <erik.herrmann@chargebyte.com>
Signed-off-by: Erik Herrmann <erik.herrmann@chargebyte.com>
Before the CAN-interface had to be configured on the operating system. Now the usage of libsocketcan allows to do this setup from EVerest-configs. Signed-off-by: Erik Herrmann <erik.herrmann@chargebyte.com>
Signed-off-by: Erik Herrmann <erik.herrmann@chargebyte.com>
Signed-off-by: Erik Herrmann <erik.herrmann@chargebyte.com>
Before a static one second wait was used to check if contactors switched correctly. Now a condition variable is used to notify immediately if a change is noticed. Signed-off-by: Erik Herrmann <erik.herrmann@chargebyte.com>
Erik827
force-pushed
the
feature/CbCpxDriver
branch
from
0a71afa to
6e2d8a7
Compare
- Rename CAN interface/bitrate config keys and propagate through driver - Rename CAN interface source/header paths and includes - Update CAN socket/member naming and helper functions for clarity - Tighten type handling with explicit bool/size casts - Tidy manifest (move device_id, add CAN bitrate bounds, drop rcm_enable) Signed-off-by: Erik Herrmann <erik.herrmann@chargebyte.com>
There was a problem hiding this comment.
I did not want you to change the variables in the manifest to be mixed-case, nor I wanted to change file names to upper case. I only wanted that you use "CAN" in comments and text.
And as mentioned before, please squash your commit, rebase on latest main branch and force-push to this branch to update this PR.
4 participants
No description provided.