Move Huawei CAN bus communication to separate thread

[MalteSchm]

Multiple issues have been raised related to Huawei AC charger connection issues (links below). These are related to the response of the Huawei PSU that delivers the individual values (actual current, voltage etc.) as separate CAN bus messages. These were previously read as part of the main() thread which used to work okisch. The changes implemented over the course of the summer however caused this code to be executed less often. This now causes frequent overrides of the small CAN receiver buffer by newer messages resulting in data loss.

This PR implements CAN communication as a separate thread, responsible for CAN bus reads and writes. This thread fetches data more often but is limited to CAN bus communication for performance reasons only. My thought was to keep the methods of the CAN bus communication class thread safe and only use it from the exiting Huawei CAN class. The rest of the OpenDTU code would continue to interact with the existing Huawei CAN class so that thread safety concerns do not need to be taken into account.

The weather is nice today, so I'll run a last test today and tomorrow (so I'm marking this as draft). The code has been in use for probably 4 weeks now.

#387
#316

[schlimmchen]

[I am currently evaluating SoftwareSerial to read the Victron MPPT data, and this PR gives me an idea that might very well make SoftwareSerial worth a serious effort, thanks!]

You use locking, very good. I did not check it in detail. I assume you interlocked every data member that the new task and the main loop might access? You might want to check out std::mutex and std::lock_guard to get rid of error-prone and oldschool xSemaphoreTake and ...Give alternatives.

Have you considered what happens if the user changes settings? Will another task be created or is this situation handled gracefully? Is the task stopped and cleaned up when the charger is disabled?

Again, I could make sure by reading the code thoroughly, but for now excuse me that I am asking possibly dumb questions, hoping to contribute useful thoughts.

[MalteSchm]

You use locking, very good. I did not check it in detail. I assume you interlocked every data member that the new task and the main loop might access? You might want to check out std::mutex and std::lock_guard to get rid of error-prone and oldschool xSemaphoreTake and ...Give alternatives.

I was not aware of std::mutex and std::lock_guard but reading a bit it seems that these are not implemented in FreeRTOS.
Apart from that data members are interlocked where needed, yes.

Have you considered what happens if the user changes settings? Will another task be created or is this situation handled gracefully? Is the task stopped and cleaned up when the charger is disabled?

Yes to some extend. The task is created in HuaweiCanClass::init() which can be executed only once as _initialized is set to true at the end of the function. This allows to enable / disable the PSU without creating extra tasks but has it's limitations. e.g. when the user re-configures Pins we'll not apply these changes until a re-start is performed. The task is never stopped and continues to read (and potentially send) data over the CAN bus. I did not consider this a problem in the first place.

Your question made me realize however that some more checks are needed if the PSU is user enabled. The current implementation allows to enable, then disable the PSU using the UI and then continue to control it e.g. via MQTT. I will push an update to fix this

Again, I could make sure by reading the code thoroughly, but for now excuse me that I am asking possibly dumb questions, hoping to contribute useful thoughts.

Always useful. Thanks a lot. I found one additional issue this weekend and addressed this and marking this ready for review now.

[schlimmchen]

I was not aware of std::mutex and std::lock_guard but reading a bit it seems that these are not implemented in FreeRTOS.

Oh yes, they are! Search the Hoymiles lib for proof 😉 (I don't know where exactly they are implemented, but they may be used in this project for sure)

[MalteSchm]

I was not aware of std::mutex and std::lock_guard but reading a bit it seems that these are not implemented in FreeRTOS.

Oh yes, they are! Search the Hoymiles lib for proof 😉 (I don't know where exactly they are implemented, but they may be used in this project for sure)

Good one. I found them :-)
PR has been updated to use std::mutexcand std::lock_guard

Please don't merge yet, testing. Seems I can't send this back to draft state

[MalteSchm]

Hi,

I ran some further tests this weekend and fixed one remaining issue where an active Hoymiles inverter was not correctly detected in full automatic mode. With this fixed and tested today I consider this ready for merging @helgeerbe

Thanks,
Malte

[helgeerbe]

Thanks @MalteSchm I will merge it into development.

[indie89]

I just installed the new version with the Huawei bus communication running in a separate thread. And what can I say: it is such a smooth communication now! Never more than 3 seconds old data. What a dream (even though beforehand it was also quite stable, it sometimes took 30-50 seconds until communication worked properly. No comparison to now. Thanks a lot!!!).

[indie89]

But communication is now not logged in the console anymore, isn't it?

[MalteSchm]

But communication is now not logged in the console anymore, isn't it?

What exactly do you mean?

I had to change things quite a bit. With regards to console printouts I had to avoid printing from the CAN communication thread as this would not be thread safe. So changes did happen and I may have overlooked something

BTW: new data is requested every 2.5s so a data age of max 3s is a consequence of this interval

[indie89]

What exactly do you mean?

I had to change things quite a bit. With regards to console printouts I had to avoid printing from the CAN communication thread as this would not be thread safe. So changes did happen and I may have overlooked something

I was talking about the printout of the communication on the console which is not available anymore (at least with me). But as the new data request interval is 2.5s it is for sure the best option, as otherwise the console would be flooded.

[schlimmchen]

With regards to console printouts I had to avoid printing from the CAN communication thread as this would not be thread safe.

MessageOutput is thread-safe 😉 See #418

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns.