Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: VE.Direct: Support for a second MPPT tracker #706

Merged
merged 2 commits into from
Mar 17, 2024

Conversation

Gumbagubanga
Copy link

Picking up the work of @schlimmchen in #505 and #516 I tried to implement the support for a second MPPT tracker.

Changes:

  • The pin mapping for victron_rx/tx was turned to an array, thus changing the pinout
  • VictronMppt got an additional function returning a vector of spData_t for use in WebApi_ws_vedirect_live, MqttHandleVedirect and MqttHandleVedirectHass
  • VeDirectMpptController::init passes the hwSerialPort up to the VeDirectFrameHandler
  • Updated VedirectView to accompliment a second tracker OpenDTU VeDirect-Homepage
  • In order to keep delta-update behaviour in MqttHandleVedirect _kvFrame became a map with the serial number as key.

Notes:

  • Changes in MqttHandleVedirectHass were not tested as I'm not running Home Assistant and honestly don't know where to look.
  • I'm not happy how the live view page behaves when one tracker is not available, misconfigured or similar. In those cases the whole section sometimes is not rendered or the delay between updates is about 2x 2 seconds. Any recommendations how to improve on that?

@Gumbagubanga Gumbagubanga marked this pull request as draft March 2, 2024 23:10
Copy link
Member

@schlimmchen schlimmchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does look promising, thanks for your efforts! 💪 There are more things that I don't like, but they are rooted in the existing design and not something that must be tackled to make this feature fly.

include/PinMapping.h Outdated Show resolved Hide resolved
include/WebApi_ws_vedirect_live.h Outdated Show resolved Hide resolved
src/MqttHandlVedirectHass.cpp Show resolved Hide resolved
src/MqttHandlVedirectHass.cpp Outdated Show resolved Hide resolved
src/PinMapping.cpp Outdated Show resolved Hide resolved
src/VictronMppt.cpp Outdated Show resolved Hide resolved
include/VictronMppt.h Outdated Show resolved Hide resolved
src/VictronMppt.cpp Outdated Show resolved Hide resolved
@schlimmchen
Copy link
Member

I'm not happy how the live view page behaves when one tracker is not available, misconfigured or similar. In those cases the whole section sometimes is not rendered or the delay between updates is about 2x 2 seconds. Any recommendations how to improve on that?

How did you make sure that the live view can handle only one set of details being present? How did you make sure that the websocket pushes the one updated or all data sets if any of the data sets was updated?

webapp_dist/js/app.js.gz Outdated Show resolved Hide resolved
@Gumbagubanga
Copy link
Author

I'm not happy how the live view page behaves when one tracker is not available, misconfigured or similar. In those cases the whole section sometimes is not rendered or the delay between updates is about 2x 2 seconds. Any recommendations how to improve on that?

How did you make sure that the live view can handle only one set of details being present? How did you make sure that the websocket pushes the one updated or all data sets if any of the data sets was updated?

On the backend side WebApi_ws_vedirect_live iterates over the controllers and creates for all available ones a new entry in the introduced json-array vedirect.devices.

I updated the data structure Vedirect in VedirectLiveDataStatus.ts accordingly to support the new array that include the existing VedirectDevice, VedirectInput and VedirectOutput.

In VedirectView.vue the existing render blocks for those three moved into a seperate ui-card and a v-for did the trick.

@schlimmchen
Copy link
Member

Yeah, and it looks okay. I think that VictronMpptClass::isDataValid() may be a culprit: it returns false if any one of the controller has bad data. This prevents the data from the working MPPT being published through MQTT if one MPPT controller is "down".

How do you feel about generating one card for each controller with its own data age and data_critical attributes, rather than trying to account for all controllers in a "container" card? This way one could better identify issues if there are any with a single MPPT controller.

There is one drawback of using getData() with all indices: Most people have only one MPPT and they will see an error message that index 1 is out of bounds.

I think we should instead be able to iterate over all controllers (in the websocket implementation, the MQTT handler, and the HASS handler). That means the controller must be managed by a shared_ptr rather than a unique_ptr and we shall give access to the controllers through a const container with const shared_ptr. I think that would solve many issues and would be a cleaner design.

This is very good work and it will find its way into the project for sure! Me, personally, I am interested in this as well, as I am planning on adding another Victron MPPT to my setup this year. Please don't be discouraged by me giving even more feedback.

What do you think must be addressed even before this can be merged? I can test the Home Assistent integration, once I figure out how to fake one or both controllers. Does the web live view work well and as before if only one MPPT is configured?

@Gumbagubanga
Copy link
Author

Yeah, and it looks okay. I think that VictronMpptClass::isDataValid() may be a culprit: it returns false if any one of the controller has bad data. This prevents the data from the working MPPT being published through MQTT if one MPPT controller is "down".

How do you feel about generating one card for each controller with its own data age and data_critical attributes, rather than trying to account for all controllers in a "container" card? This way one could better identify issues if there are any with a single MPPT controller.

There is one drawback of using getData() with all indices: Most people have only one MPPT and they will see an error message that index 1 is out of bounds.

I think we should instead be able to iterate over all controllers (in the websocket implementation, the MQTT handler, and the HASS handler). That means the controller must be managed by a shared_ptr rather than a unique_ptr and we shall give access to the controllers through a const container with const shared_ptr. I think that would solve many issues and would be a cleaner design.

This is very good work and it will find its way into the project for sure! Me, personally, I am interested in this as well, as I am planning on adding another Victron MPPT to my setup this year. Please don't be discouraged by me giving even more feedback.

What do you think must be addressed even before this can be merged? I can test the Home Assistent integration, once I figure out how to fake one or both controllers. Does the web live view work well and as before if only one MPPT is configured?

No worries!
I'm thankful for the feedback and looking for a good rather than a quick solution.

As C++ is not my main language and I can only work on pet projects in my free time after work, it might just take a moment longer until I'd call s.th. finished. So thank you for your patience :-)

@Gumbagubanga
Copy link
Author

About faking victrons: I'm currently using two USB-TTL uart adapter on a rpi directly connected to a ESP32 as well as this emulator . Works great as mock setup.

@rs3hc
Copy link

rs3hc commented Mar 10, 2024

Not sure if it's a helping comment, but I already use 2 100/20 on a single ESP with one RX wire per MPPT.
It works great.

https://github.com/KinDR007/VictronMPPT-ESPHOME

Not sure how difficult it is to get a free pin for this in the project 🤷‍♂️

@Gumbagubanga and @schlimmchen I really appreciate your work 👍👍👍

@Gumbagubanga
Copy link
Author

Gumbagubanga commented Mar 10, 2024

Yeah, and it looks okay. I think that VictronMpptClass::isDataValid() may be a culprit: it returns false if any one of the controller has bad data. This prevents the data from the working MPPT being published through MQTT if one MPPT controller is "down".

I did some changes on that. ATM I'd say it's a patch and not a solution by individually checking on the validity of the data.

How do you feel about generating one card for each controller with its own data age and data_critical attributes, rather than trying to account for all controllers in a "container" card? This way one could better identify issues if there are any with a single MPPT controller.

I'm not sure about the change towards two cards as this doubles the DPL indicator.

Vedirect with two cards

There is one drawback of using getData() with all indices: Most people have only one MPPT and they will see an error message that index 1 is out of bounds.

Publishing the vector of spData_t was my take on that issue so that only valid data can be accessed in the first place.

I think we should instead be able to iterate over all controllers (in the websocket implementation, the MQTT handler, and the HASS handler). That means the controller must be managed by a shared_ptr rather than a unique_ptr and we shall give access to the controllers through a const container with const shared_ptr. I think that would solve many issues and would be a cleaner design.

The switch to a shared_ptr I get. But I lack the imagination/C++ know-how for the const-container. Can you give me some more details? I believe the shared_ptr on the container is there for memory-management reasons!?

Overall I looked over the code several times this week and I got the feeling that the VictronMppt class is here to encapsulate the handling of the controllers and not exposing them to the outside (i.e. MQTT, HASS, WS-Live-View). Would the container not archive the contrary?

Another issue I found is with the hwserials. I don't know which hardware this project will support in the future, but managing the uart ports over all implementations might get messy and thus a cross cutting concern.
See, the already supported ESP32-S2 has only 2 ports according to espressif's documentation.

Edit: Obsolete

On the one hand I want to tackle this issue but on the on the other hand I see the holistic solution not part of this PR. So warnings will be the solution here.

@Gumbagubanga
Copy link
Author

Not sure if it's a helping comment, but I already use 2 100/20 on a single ESP with one RX wire per MPPT. It works great.

https://github.com/KinDR007/VictronMPPT-ESPHOME

Not sure how difficult it is to get a free pin for this in the project 🤷‍♂️

@Gumbagubanga and @schlimmchen I really appreciate your work 👍👍👍

Thanks @rs3hc for the link to this project. 👍

Actually OpenDTUonBattery does have all the code to access multiple Victron MPPTs.
Aside from exposing the configuration and updating all relevant references, we have to manage the available uart ports the ESP32 has (3 ports on the original and S3, 2 ports on the S2).
As this project supports more hardware that might be connected by a user to the board, they cannot be used all at once.

@Gumbagubanga
Copy link
Author

Gumbagubanga commented Mar 10, 2024

I forgot to remove the sub-card I previously introduced.
Vedirect with two cards

@SW-Niko
Copy link

SW-Niko commented Mar 12, 2024

Hello @Gumbagubanga,
I like this feature. If you need someone for testing, you can count on me.

Does this feature have an impact on the inverter control or is the data from the second MPPT just displayed?

@schlimmchen
Copy link
Member

Does this feature have an impact on the inverter control or is the data from the second MPPT just displayed?

You are probably asking about solar passthrough. Yes, solar passthrough will then consider the output of both Victron MPPT charge controllers.

I'm not sure about the change towards two cards as this doubles the DPL indicator.

Good point. The DPL desperately needs its own card. Let's not postpone it any longer. The card will be very small at the start, but let's get it done. We can then add more values to it later. Then we are free to produce as many VE.Direct cards as we please. I can tackle this. Let's do it after this is merged, so be don't cause trouble rebasing your work. It's acceptable that the second card duplicates the DPL state for some time.

@Gumbagubanga
Copy link
Author

Currently DPL won't work, if in a two MPPT scenario one of them is returning invalid data.

Should I change implementation of VictronMppt.isDataValid() so that it will return true, if at least one valid MPPT is found?

@schlimmchen
Copy link
Member

Should I change implementation of VictronMppt.isDataValid() so that it will return true, if at least one valid MPPT is found?

Only if you take care that the methods iterating the controllers always check their respective "isDataValid()" methods and only use their values if the data is valid. Example: Total solar power is requested. Iterate all controllers. Only add values of controller with valid data, otherwise the calculated solar power will use outdated data, which might be too high.

We might also need to check if isDataValid() really returns false after a while when breaking the connection to a charge controller. AFAIK if the data is stale, it is valid forever. That should not be the case.

This changeset is becoming quite large. You are putting a lot of work into it, which is great, but the changes are becoming very substantial and hard to review. Also, I would like to start testing this and planned to merge this soon. What else do you plan to do before we can attempt a merge and release?

@schlimmchen schlimmchen self-assigned this Mar 15, 2024
@Gumbagubanga
Copy link
Author

Should I change implementation of VictronMppt.isDataValid() so that it will return true, if at least one valid MPPT is found?

Only if you take care that the methods iterating the controllers always check their respective "isDataValid()" methods and only use their values if the data is valid. Example: Total solar power is requested. Iterate all controllers. Only add values of controller with valid data, otherwise the calculated solar power will use outdated data, which might be too high.

We might also need to check if isDataValid() really returns false after a while when breaking the connection to a charge controller. AFAIK if the data is stale, it is valid forever. That should not be the case.

This changeset is becoming quite large. You are putting a lot of work into it, which is great, but the changes are becoming very substantial and hard to review. Also, I would like to start testing this and planned to merge this soon. What else do you plan to do before we can attempt a merge and release?

I think the hardest parts have been tackled to get a stable 2 MPPT setup running. Unless you recommend improving on particular aspects, I'd say "let the testing begin".

Following issues can be handled by new PRs:

  • DPL backend refinement: As the wired connection between the ESP32 and the MPPTs aren't flacky, the above mentioned issue shouldn't be that bad. Reverting back to a single MPPT setup is always an option.
  • DPL frontend refinement: The DPL needs an own card on the live page or at least a better place to live.
  • VictronMppt#getData(idx) and #isDataValid(idx): I'm not happy with it but it's functional. I'd like to invest more time in refactoring later on.

@SW-Niko
Copy link

SW-Niko commented Mar 15, 2024

I want to share few thoughts about “How to merge the data from the 2 MPPTs”

Clear on total power. Just adding the two values. But what happens to the other values used for regulation?

The MPPT voltage can be used for the inverter switch-off threshold and for the solar passthrough mode. Which of the two values do you take? The smaller voltage? The greater voltage? Average?
My opinion: You should use the MPPT voltage that best approximates the battery voltage. But this can vary from system to system.

That's why I propose a simple transparent solution. The voltage from the MPPT which is connected to serial port 1 is always used for the “battery voltage”. The installer can then use the MPPT that is best for them just by VE.Direct wiring to serial port 1.
Port 1 always the Master MPPT and Port 2 always the Slave MPPT.
Alternatively, you could also offer a selection switch in the web interface. But I wouldn't make that extra effort.

Apart from the MPPT power and the MPPT voltage, are there any other parameters that are used for regulation/controlling?

@rs3hc
Copy link

rs3hc commented Mar 15, 2024

@Gumbagubanga or @schlimmchen
Do you have a actual pin mapping for the second mppt support?

@schlimmchen
Copy link
Member

Which of the two values do you take?

https://github.com/helgeerbe/OpenDTU-OnBattery/blob/2efa1b35b0676509c7f321ccea48724f9fc4e31a/src/VictronMppt.cpp#L141-L152

It's the minimum voltage.

Do you have a actual pin mapping for the second mppt support?

It's victron.rx2 and victron.tx2, analogue to victron.rx and victron.tx.

@Gumbagubanga Gumbagubanga marked this pull request as ready for review March 15, 2024 21:12
@SW-Niko
Copy link

SW-Niko commented Mar 16, 2024

Hello @schlimmchen ,
ok, thanks for the information.
PS: I'm on the way to prepare my system for the second VE.Direct port. Need to add some hardware.

@schlimmchen
Copy link
Member

image

The cpplinter is on my side... Going to revert the indention in JkBms namespace.

Gumbagubanga and others added 2 commits March 17, 2024 16:42
this change adds support for a second Victron MPPT charge controller
using a second serial connection.

* Add device configuration for a second victron mppt
* Update VedirectView for second victron mppt
* Update MqttHandleVedirect for second victron mppt
* Update MqttHandleVedirectHass for second victron mppt
* Handle nonexisting victron controllers with optionals
* Add bool-function to Battery and inherited classes, if uart port 2 is
  being used
* Introduced a serial port manager. In order to prevent the battery and
  the Victron MPPT to use the same hw serial ports, this class keeps
  track of the used ports and their owners.
* fix compiler warning in SerialPortManager.cpp: function must not
  return void

* clean up and simplify implementation of usesHwPort2()
  * make const
  * overrides are final
  * default implementation returns false
  * implement in header, as the implementation is very simple

* rename PortManager to SerialPortManager. as "PortManager" is too
  generic, the static instance of the serial port manager is renamed to
  "SerialPortManager". the class is therefore renamed to
  SerialPortManagerClass, which is in line with other (static) classes
  withing OpenDTU(-OnBattery).

* implement separate data ages for MPPT charge controllers

* make sure MPPT data and live data time out

* do not use invalid data of MPPT controlers for calculations

* add :key binding to v-for iterating over MPPT instances
Copy link
Member

@schlimmchen schlimmchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@schlimmchen schlimmchen merged commit 7d6b725 into hoylabs:development Mar 17, 2024
8 checks passed
@rs3hc
Copy link

rs3hc commented Mar 21, 2024

Which of the two values do you take?

https://github.com/helgeerbe/OpenDTU-OnBattery/blob/2efa1b35b0676509c7f321ccea48724f9fc4e31a/src/VictronMppt.cpp#L141-L152

It's the minimum voltage.

Do you have a actual pin mapping for the second mppt support?

It's victron.rx2 and victron.tx2, analogue to victron.rx and victron.tx.

Sorry für die evtl blöde Frage.

Welchen Pin nutzt ihr für den zweiten MPPT/VE.Direct Eingang?
Oder gibt es dafür Einschränkungen welche nutzbar sind?

Liebe Grüße aus Sachsen 👍

@schlimmchen
Copy link
Member

Ja, naja... Die Frage ist so mittelmäßig qualifiziert 😁

Es eignet sich jeder Pin.

Einschränkungen gibt es nur insofern, als dass manche ESP32 Pins "Input only" sind. Da du aber einen Pin als Input brauchst, ist es tatsächlich egal. Es muss natürlich ein freier GPIO sein, das ist klar. Und nochmal die Erinnerung: Du solltest galvanisch trennen, z.B. mit einem ADUM1201. Zu diesem Thema findest du reichlich Infos im Netzs oder in den Diskussionen hier.

@rs3hc
Copy link

rs3hc commented Mar 21, 2024

Ja, naja... Die Frage ist so mittelmäßig qualifiziert 😁

Es eignet sich jeder Pin.

Einschränkungen gibt es nur insofern, als dass manche ESP32 Pins "Input only" sind. Da du aber einen Pin als Input brauchst, ist es tatsächlich egal. Es muss natürlich ein freier GPIO sein, das ist klar. Und nochmal die Erinnerung: Du solltest galvanisch trennen, z.B. mit einem ADUM1201. Zu diesem Thema findest du reichlich Infos im Netzs oder in den Diskussionen hier.

Ja ist schon gut mit der Kritik 😁

Ich versuche nur dem Alex Seufert bei seinem Board zu unterstützen. (https://www.akkudoktor.net/forum/migrated-forums-balkonsolar/opendtu-on-battery-platine-mit-maximal-ausbau-dc-dc-und-jk-bms-stecker/paged/3/)

Mit ESP habe ich nur einfache Bastelkenntnisse, spätestens beim Thema UART bin ich fachlich raus.
Mein Steckenpferd sind USVs mit riesigen Batterieanlagen.

Kurz und Knapp.
Ist es, von der Software aus möglich das so zum Laufen zu bekommen?
Bzw. wo liegen aktuell die Fallstricke?

Ich:
2 VE.Direct MPPTs , 1 SmartShunt, CMT2300A und nach Möglichkeit ein SH1106 Display

Nachbar:
2 VE.Direct MPPT, 1 UC5000 über RS485, CMT2300A und nach Möglichkeit ein SH1106 Display

Danke für eure Arbeit 👍

@schlimmchen
Copy link
Member

2 VE.Direct MPPTs , 1 SmartShunt, CMT2300A und nach Möglichkeit ein SH1106 Display

Nur mit ESP32-S3 (wie auf OpenDTU-Fusion Board) und nur mit weiterer (von mir geplanter) Unterstützung in Software, denn du brauchst ja 3 UARTs. Der S3 hat drei zur Verfügung (weil die "Konsole" über USB direkt angehängt ist), aber das muss alles erst noch nutzbar gemacht werden. Ohne einen ESP32 mit 3 UARTs geht es erst, wenn auch die Unterstützung für ein SoftwareSerial drin ist.

2 VE.Direct MPPT, 1 UC5000 über RS485, CMT2300A und nach Möglichkeit ein SH1106 Display

Für eine Pylontech an RS485 gibt es soweit ich weiß keine Softwareunterstützung. Und wenn du CAN nimmst, kollidiert das soweit ich mich erinnere mit dem CMT2300A SPI? Bin mir nicht sicher...

Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants