Integration of Victron SmartShunt via VE.Direct

[philippsandhaus]

This a first draft of my integration as discussed in #205. As I did quite a lot of restructuring and I am not that confident in C++ I am marking this as draft and hereby ask for feedback from @schlimmchen and/or @helgeerbe. I rebased the current status on top of the development branch.

[helgeerbe]

@philippsandhaus good job! I just looked at the frame handler. I would propose to use a VeDirectControllerbase class and derive veDirectShuntControllerand VeDirectMpptControllerfrom it.

Advantage is, that you can do the generic stuff like init of the HardwareSerial in the base class and you have to implement it only once. Do only device specific handling in the DeviceController class.

[schlimmchen]

VeDirectFrameHandler is the new base class. @helgeerbe He did not do more of init() in the base class because the derived classes use different instances of HardwareSerial. However, this could still be optimized: Pass 1 or 2 from the derived classes init() to the base class init() to construct HardwareSerial in the base class.

webapp_dist should not be part of a pull request. @helgeerbe will recompile the webapp before tagging a release, which then includes all changes to the webapp that have been merged in the meantime.

I did a review with a bunch of comments, but only minor things. I really like how this turned out! Maybe I can merge this into my devel branch and test soon. @philippsandhaus You did test that the MPPT interface is working as before, correct?

[philippsandhaus]

Thanks for the review. I adjusted my code accordingly and also moved initialization of the HardwareSerial interface to the base class. As far as I know there are only 2 HardwareSerial interfaces on the ESP32, right? So if there are more than 2 serial devices, one would have to think about some kind of management and also use SoftwareSerial, I guess.

Yes, for me the MPPT interface still works as before.

[schlimmchen]

As far as I know there are only 2 HardwareSerial interfaces on the ESP32, right? So if there are more than 2 serial devices, one would have to think about some kind of management and also use SoftwareSerial, I guess.

You are absolutely right. There technically are 3, if I remember correctly, but one is used for the serial console. Trying SoftwareSerial for VE.Direct ist on my TODO list for weeks now... it should work as the baudrate is moderate.

[helgeerbe]

I wouldn't mind, if we switch to Software Serial completely for ve.direct devices. Guess this makes the orchestration easier. @schlimmchen pointed out baudrate is moderate and the actual loop time is fast enough.

Just thinking loud, if we are going to support also a second charger.

[philippsandhaus]

SoftwareSerial and HardwareSerial both derive from Stream. I envision a class like SerialProvider which provides instances of Stream with for example SerialProvider.getSerial(19200, SERIAL_8N1, rx, tx). 'SerialProvider' could then make the decision to hand an instance of HardwareSerial or SoftwareSerial based on baudrate and/or availability of hardware serial ports. But I guess the tricky parts are the details like how to get notified when a Hardware Serial port is freed up.

[schlimmchen]

@philippsandhaus Interesting thoughts! How about you open up a new issue where we can discuss this (rather than being buried in this PR)?

[schlimmchen]

@philippsandhaus I see there are values in veShuntStruct that are of type double. However, all values are always converted using atoi. I think it's fine to save the values as they were on the serial line, i.e., integers, and to do the scaling later -- even though this is different than in the MPPT counterpart. You do the scaling in VictronSmartShuntStats::updateFrom. If so, shouldn't the values in the veShuntStruct be all int32_t?

[schlimmchen]

@philippsandhaus Feel free to cherry pick from https://github.com/schlimmchen/OpenDTU-OnBattery/commits/devel for this PR -- if you like. And except for the SoftwareSerial commit, that's a work in progress and requires testing.

[philippsandhaus]

Great, I have cherry picked most of your adjustments. I also added handling of alarm cases for the live view. At least for me this PR would now be complete.

[schlimmchen]

At least for me this PR would now be complete.

Affirmative.

[helgeerbe]

Cool. I could merge it into development. But I'm away from keyboard next week. So I would keep it in development for one additional week for further testing, before I release it. Are you fine with that?

[swingstate]

I'll install a SmartShunt 500A tomorrow morning and was assuming you're going to release, however no worries - I'll compile development and flash that to my DTU for testing. Will report any findings and if no issue report back a "no issues"

[philippsandhaus]

Cool. I could merge it into development. But I'm away from keyboard next week. So I would keep it in development for one additional week for further testing, before I release it. Are you fine with that?

Sure, no problem.

[philippsandhaus]

I'll install a SmartShunt 500A tomorrow morning and was assuming you're going to release, however no worries - I'll compile development and flash that to my DTU for testing. Will report any findings and if no issue report back a "no issues"

The changes are not in the development branch yet. But you can take a snapshot build from my repo https://github.com/philippsandhaus/OpenDTU-OnBattery/actions/runs/6276010329

[swingstate]

guess now it is. I'll compile and flash my ESP tomorrow morning.

[helgeerbe]

It's in development now. And it's on my OpenDTU.
Happy testing!

[schlimmchen]

Whoopsie... @helgeerbe Please double-check if this commit can stay in your repo: b03bc9b @philippsandhaus pushed it right before you merged this PR.

[helgeerbe]

I removed the line. From my point of view. You should do the build locally.
Action is only used to ensure, that every commit to development results in code that is compilable.

[swingstate]

@helgeerbe I have the code on my ESP now and so far things look good, SOC is being displayed. I have one unclarity which I shared in the longer issue thread #205.

[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.