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

Esp32 restart by activating VE direkt option #481

Closed
MacGyver23 opened this issue Oct 5, 2023 · 24 comments · Fixed by #505
Closed

Esp32 restart by activating VE direkt option #481

MacGyver23 opened this issue Oct 5, 2023 · 24 comments · Fixed by #505
Labels
bug Something isn't working

Comments

@MacGyver23
Copy link

What happened?

With the new firmware my ESP32 BOARD restart (reset) when activated the VE direkt option ...i canot activat this funktion.

To Reproduce Bug

Activate VEDirekt option

Expected Behavior

No resett device by activating VE direkt option

Install Method

Pre-Compiled binary from GitHub

What git-hash/version of OpenDTU?

4324ae3

Relevant log/trace output

No response

Anything else?

No response

@MacGyver23 MacGyver23 added the bug Something isn't working label Oct 5, 2023
@philippsandhaus
Copy link

Did it work before upgrading? And did you correctly define the GPIO Pins for your Victron devices? Could you share your pin mapping here?

@schlimmchen
Copy link
Member

Also please capture the device's UART output (using USB and your computer) when the device starts or when it produces the error and share the output.

@MacGyver23
Copy link
Author

I use the version from 21.6.2023 because the GPIO for the victron is correctly specified there.
In the later versions only the NRF24 is configured and the PinMapping.json from the version of 21.6.2023 does not work in later versions.
In the later version there were not the crashes.
In the latest version, a factory reset is performed when the VE.Direkt option is activated.

I cannot edit the old PinMapping.json file because something is wrong with the formatting.

I don't know how to create a new PinMapping.json, I'm not that experienced.

Is there a PinMapping.json file somewhere for download?

Sorry for the long text

@philippsandhaus
Copy link

You could go to Settings -> Config Management with the newest version and select pin_mapping.json at the top from the drop-down list. You can download a working file with Backup then. Then try to edit this file and restore it.

@MacGyver23
Copy link
Author

Ok thanks for the info.
I'll try it and get back to you about the crashes when activating the VE.direct function.

Does the esp32 crash because no GPIO pins are assigned for the victron?

@MacGyver23
Copy link
Author

I have tried it with the latest firmware , there is no PinMapping.json to find only a config.jason

I use a ESP32-WROOM-32 board from AZ-Delivery with the yellow headerpins...so far without problems have 3 pieces with all the same problem...

Oh I used the opendtu on battery generic.bin

@MacGyver23
Copy link
Author

With the previous version 2023.09.28 ( f0a55ea).
It restarts as soon as ve.direct is enabled and when it is restarted it is disabled again....
And also in this version is no pinmapping file at settings config management to find ...only a config.jason no pinmapping...

@MacGyver23
Copy link
Author

The version from 2023.09.19 (ae39232) runs without crashing when activating the VE.direct function...
Geflasht I have the opendtu onbattery-generic.actory.bin

Nu unfortunately I am missing the pinmapping.jason...It does not exist in any of the 3 last version...only the config.jason

@philippsandhaus
Copy link

Are you sure? Please see the following screenshot on how to select the pin_mapping.json to backup:
Bildschirmfoto 2023-10-06 um 00 00 38

@MacGyver23
Copy link
Author

Yes i am sure
Screenshot_20231006_002022_Opera

@MacGyver23
Copy link
Author

This is my ESP32 Board
20231006_003415

@MacGyver23
Copy link
Author

This board is the same problem it crash wenn activat VE.direct option..and the pin mapping is also missing
by the last firmware
20231006_004825

@schlimmchen
Copy link
Member

Some clarification: The pin_mapping.json is part of the filesystem, not part of the firmware. On a new flash, the filesystem is empty. File config.json is created from defaults if there is none. The pin_mapping.json is not created from defaults. If there is no such file, the compiled-in default values are used. More and more of those are becoming -1 to force users to upload their own version of the file, one that fits their actual hardware setup.

Two things should happen:

  1. OpenDTU-OnBattery should not crash if the pin_mapping.json is unavailable.
  2. You have to upload a suitable pin_mapping.json yourself. Please do not rely on compiled-in defaults. See the Wiki and the examples.

@MacGyver23
Copy link
Author

Ok thanks for the info

The current firmware crashes as soon as I enable VE.direct option, after the ESP32 is rebooted this function is disabled again, so there seems to be an error

I will have a look at creating a PinMapping.json file then the victron should run again.

@philippsandhaus
Copy link

philippsandhaus commented Oct 6, 2023

Ah, thanks @schlimmchen . Honestly I was not aware that the pin_mapping.json is not created on a fresh install. But this also means that that the default the default setting is indeed -1 for the Victron rx and tx. This can very well lead to a crash when initialising the VE.Direct option (for a MPPT Charger). Activating VE.Direct without specifying the correct GPIO-Pins (or at least the rx pin) makes no sense at moment. You have to define it by uploading a corresponding pin_mapping.json file.

@MacGyver23
Copy link
Author

Ok guys problem solved it was fact of not existing PinMapping.jason that's why the new firmware crashes.
I have now created a PinMapping.json,(thanks to you guys for the great support) now everything is running again without problems with the latest version.

I was just too stupid to create the pinmapping :-)

Topic can be closed

Thanks for your great help

@schlimmchen
Copy link
Member

Nice that you got it working again.

Thanks for your great help

You're welcome.

However, please re-open the issue. The software must not crash if the pin_mapping.json does not exist. That should be handled gracefully. That still needs to be fixed.

@MacGyver23 MacGyver23 reopened this Oct 6, 2023
@MacGyver23
Copy link
Author

Ok have opened it again.

Am so happy that I got it done thanks to your help...Was forever on the version of 21.6.2023 because of the PinMapping.json did not know how to create it;-)

Theme is open again

@schlimmchen
Copy link
Member

I was able to reproduce this:

  • Erase flash of ESP32 (to remove pin_mapping.json)
  • Enable VE.Direct
[180275][E][vfs_api.cpp:105] open(): /littlefs/pin_mapping.json does not exist, no permits for creation
Guru Meditation Error: Core  1 panic'ed (LoadProhibited). Exception was unhandled.

Core  1 register dump:
PC      : 0x4205b915  PS      : 0x00060f30  A0      : 0x8203c9c5  A1      : 0x3fcebeb0  
A2      : 0x3fca2590  A3      : 0x3fc9d300  A4      : 0x00000120  A5      : 0x00000000  
A6      : 0x3fc9c8e0  A7      : 0x00000003  A8      : 0x82063b95  A9      : 0x3fcebe90  
A10     : 0x00000000  A11     : 0x00060f20  A12     : 0x4037dcef  A13     : 0x3fcebe60  
A14     : 0x3fcefa98  A15     : 0x3fca4908  SAR     : 0x0000001b  EXCCAUSE: 0x0000001c  
EXCVADDR: 0x00000000  LBEG    : 0x403813e8  LEND    : 0x403813f6  LCOUNT  : 0x00000001  

Backtrace: 0x4205b912:0x3fcebeb0 0x4203c9c2:0x3fcebed0 0x42065a2d:0x3fcebef0

ELF file SHA256: 1835a622aa67c7bb

E (17798) esp_core_dump_flash: Core dump flash config is corrupted! CRC=0x7bd5c66f instead of 0x0
Rebooting...

It seems to be a nullptr exception. This is the decoded backtrace:

0x4205b912: VeDirectFrameHandler::loop() at /home/schlimmchen/Documents/OpenDTU-OnBattery/lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp:103
0x4203c9c2: loop() at /home/schlimmchen/Documents/OpenDTU-OnBattery/src/main.cpp:207
0x42065a2d: loopTask(void*) at /home/schlimmchen/.platformio/packages/[email protected]/cores/esp32/main.cpp:50

The problem is this: VE.Direct MPPT controller is not initialized as the pin config is invalid. However, as soon as VE.Direct is enabled, the MPPT controller loop() is called from inside the main loop(). However, _vedirectSerial is still nullptr and not tested if it is.

In my opinion, this should be solved like this: Similar to VictronSmartShunt.cpp, there should be a VictronMppt.cpp. That one provides a loop() that is called unconditionally from the main loop(). This resolves the issue that VeDirectMppt is currently handled differently than all other loop()s. This new VictronMppt.cpp knows the config and manages one (or in the future multiple) instances of VeDirectMpptController. This solves another problem: (static) memory usage of a VeDirectMpptController instance for people that do not use VE.Direct at all.

Let's see if this works out, I'll start working on this and I'll open a pull request.

@philippsandhaus
Copy link

philippsandhaus commented Oct 7, 2023

A alternative quick fix would be to add something like this at the beginning of loop:

if (!_vedirectSerial) {
    return;
}

But your solution is obviously much cleaner and provides a good basis for multiple instances of VeDirectMpptController.

@schlimmchen
Copy link
Member

Yes, that would probably be sufficient. Such a patch, however, ignores that the design is broken, only addressing symptoms.

I have a draft ready, I know that it solves the issue at hand, and it prepares for support of multiple MPPT, but I have to test it thoroughly.

@schne-ste
Copy link

Same problem for me...
After downgrading to 2023.09.04 it works fine

schlimmchen added a commit to schlimmchen/OpenDTU-OnBattery that referenced this issue Oct 9, 2023
this solves a design issue where the loop() method of a static instance
of VeDirectMpptController, which is part of library code, is called as
part of the main loop() implementation. that is a problem because the
call to this loop() must be handled differently from all other calls:
the lib does not know whether or not the feature is enabled at all.
also, the instance would not be initialized when enabling the feature
during normal operation. that would even lead to a nullptr exception
since the pointer to the serial implementation is still uninitialized.

this new intermediate class is implemented with the support for multiple
Victron charge controllers in mind. adding support for more charge
controllers should be more viable than ever.

fixes hoylabs#481.

related to hoylabs#397 hoylabs#129.
schlimmchen added a commit to schlimmchen/OpenDTU-OnBattery that referenced this issue Oct 16, 2023
this solves a design issue where the loop() method of a static instance
of VeDirectMpptController, which is part of library code, is called as
part of the main loop() implementation. that is a problem because the
call to this loop() must be handled differently from all other calls:
the lib does not know whether or not the feature is enabled at all.
also, the instance would not be initialized when enabling the feature
during normal operation. that would even lead to a nullptr exception
since the pointer to the serial implementation is still uninitialized.

this new intermediate class is implemented with the support for multiple
Victron charge controllers in mind. adding support for more charge
controllers should be more viable than ever.

fixes hoylabs#481.

related to hoylabs#397 hoylabs#129.
schlimmchen added a commit to schlimmchen/OpenDTU-OnBattery that referenced this issue Oct 16, 2023
this solves a design issue where the loop() method of a static instance
of VeDirectMpptController, which is part of library code, is called as
part of the main loop() implementation. that is a problem because the
call to this loop() must be handled differently from all other calls:
the lib does not know whether or not the feature is enabled at all.
also, the instance would not be initialized when enabling the feature
during normal operation. that would even lead to a nullptr exception
since the pointer to the serial implementation is still uninitialized.

this new intermediate class is implemented with the support for multiple
Victron charge controllers in mind. adding support for more charge
controllers should be more viable than ever.

fixes hoylabs#481.

related to hoylabs#397 hoylabs#129.
schlimmchen added a commit to schlimmchen/OpenDTU-OnBattery that referenced this issue Oct 16, 2023
this solves a design issue where the loop() method of a static instance
of VeDirectMpptController, which is part of library code, is called as
part of the main loop() implementation. that is a problem because the
call to this loop() must be handled differently from all other calls:
the lib does not know whether or not the feature is enabled at all.
also, the instance would not be initialized when enabling the feature
during normal operation. that would even lead to a nullptr exception
since the pointer to the serial implementation is still uninitialized.

this new intermediate class is implemented with the support for multiple
Victron charge controllers in mind. adding support for more charge
controllers should be more viable than ever.

fixes hoylabs#481.

related to hoylabs#397 hoylabs#129.
@schlimmchen
Copy link
Member

See #505 (one aspect of that PR is fixing this issue by addressing the underlying design issue).

helgeerbe pushed a commit that referenced this issue Oct 19, 2023
…es (#505)

* introduce VictronMpptClass

this solves a design issue where the loop() method of a static instance
of VeDirectMpptController, which is part of library code, is called as
part of the main loop() implementation. that is a problem because the
call to this loop() must be handled differently from all other calls:
the lib does not know whether or not the feature is enabled at all.
also, the instance would not be initialized when enabling the feature
during normal operation. that would even lead to a nullptr exception
since the pointer to the serial implementation is still uninitialized.

this new intermediate class is implemented with the support for multiple
Victron charge controllers in mind. adding support for more charge
controllers should be more viable than ever.

fixes #481.

related to #397 #129.

* VE.Direct: move get.*AsString methods to respective structs

those structs, which hold the data to be translated into strings, know
best how to translate them. this change also simplifies access to those
translation, as no parameter must be handed to the respective methods:
they now act upon the data of the instance they are called for. adds
constness to those methods.

* VE.Direct: simplify and clean up get.*AsString methods

use a map, which is much easier to maintain and which reads much easier.
move the strings to flash memory to save RAM.

* DPL: use VictronMpptClass::getPowerOutputWatts method

remove redundant calculation of output power from DPL. consider
separation of concern: VictronMpptClass will provide the total solar
output power. the DPL shall not concern itself about how that value is
calculated and it certainly should be unaware about how many MPPT charge
controllers there actually are.

* VE.Direct: avoid shadowing struct member "P"

P was part of the base struct for both MPPT and SmartShunt controller.
however, P was also part of the SmartShunt controller data struct,
shadowing the member in the base struct.

since P has slightly different meaning in MPPT versus SmartShunt, and
since P is calculated for MPPT controllers but read from SmartShunts, P
now lives in both derived structs, but not in the base struct.

* VE.Direct: isDataValid(): avoid copying data structs

pass a const reference to the base class implementation of isDataValid()
rather than a copy of the whole struct.

* VE.Direct: unify logging of text events

* VE.Direct: stop processing text event if handled by base

in case the base class processed a text event, do not try to match it
against values that are only valid in the derived class -- none will
match.

* VE.Direct MPPT: manage data in a shared_ptr

instead of handing out a reference to a struct which is part of a class
instance that may disappear, e.g., on a config change, we now manage the
lifetime of said data structure using a shared_ptr and hand out copies
of that shared_ptr. this makes sure that users have a valid copy of the
data as long as they hold the shared_ptr.

* VE.Direct MPPT: implement getDataAgeMillis()

this works even if millis() wraps around.

* VE.Direct: process frame end event only for valid frames

save a parameters, save a level of indention, save a function call for
invalid frames.
philippsandhaus pushed a commit to philippsandhaus/OpenDTU-OnBattery that referenced this issue Oct 20, 2023
…es (hoylabs#505)

* introduce VictronMpptClass

this solves a design issue where the loop() method of a static instance
of VeDirectMpptController, which is part of library code, is called as
part of the main loop() implementation. that is a problem because the
call to this loop() must be handled differently from all other calls:
the lib does not know whether or not the feature is enabled at all.
also, the instance would not be initialized when enabling the feature
during normal operation. that would even lead to a nullptr exception
since the pointer to the serial implementation is still uninitialized.

this new intermediate class is implemented with the support for multiple
Victron charge controllers in mind. adding support for more charge
controllers should be more viable than ever.

fixes hoylabs#481.

related to hoylabs#397 hoylabs#129.

* VE.Direct: move get.*AsString methods to respective structs

those structs, which hold the data to be translated into strings, know
best how to translate them. this change also simplifies access to those
translation, as no parameter must be handed to the respective methods:
they now act upon the data of the instance they are called for. adds
constness to those methods.

* VE.Direct: simplify and clean up get.*AsString methods

use a map, which is much easier to maintain and which reads much easier.
move the strings to flash memory to save RAM.

* DPL: use VictronMpptClass::getPowerOutputWatts method

remove redundant calculation of output power from DPL. consider
separation of concern: VictronMpptClass will provide the total solar
output power. the DPL shall not concern itself about how that value is
calculated and it certainly should be unaware about how many MPPT charge
controllers there actually are.

* VE.Direct: avoid shadowing struct member "P"

P was part of the base struct for both MPPT and SmartShunt controller.
however, P was also part of the SmartShunt controller data struct,
shadowing the member in the base struct.

since P has slightly different meaning in MPPT versus SmartShunt, and
since P is calculated for MPPT controllers but read from SmartShunts, P
now lives in both derived structs, but not in the base struct.

* VE.Direct: isDataValid(): avoid copying data structs

pass a const reference to the base class implementation of isDataValid()
rather than a copy of the whole struct.

* VE.Direct: unify logging of text events

* VE.Direct: stop processing text event if handled by base

in case the base class processed a text event, do not try to match it
against values that are only valid in the derived class -- none will
match.

* VE.Direct MPPT: manage data in a shared_ptr

instead of handing out a reference to a struct which is part of a class
instance that may disappear, e.g., on a config change, we now manage the
lifetime of said data structure using a shared_ptr and hand out copies
of that shared_ptr. this makes sure that users have a valid copy of the
data as long as they hold the shared_ptr.

* VE.Direct MPPT: implement getDataAgeMillis()

this works even if millis() wraps around.

* VE.Direct: process frame end event only for valid frames

save a parameters, save a level of indention, save a function call for
invalid frames.
philippsandhaus pushed a commit to philippsandhaus/OpenDTU-OnBattery that referenced this issue Oct 20, 2023
…es (hoylabs#505)

* introduce VictronMpptClass

this solves a design issue where the loop() method of a static instance
of VeDirectMpptController, which is part of library code, is called as
part of the main loop() implementation. that is a problem because the
call to this loop() must be handled differently from all other calls:
the lib does not know whether or not the feature is enabled at all.
also, the instance would not be initialized when enabling the feature
during normal operation. that would even lead to a nullptr exception
since the pointer to the serial implementation is still uninitialized.

this new intermediate class is implemented with the support for multiple
Victron charge controllers in mind. adding support for more charge
controllers should be more viable than ever.

fixes hoylabs#481.

related to hoylabs#397 hoylabs#129.

* VE.Direct: move get.*AsString methods to respective structs

those structs, which hold the data to be translated into strings, know
best how to translate them. this change also simplifies access to those
translation, as no parameter must be handed to the respective methods:
they now act upon the data of the instance they are called for. adds
constness to those methods.

* VE.Direct: simplify and clean up get.*AsString methods

use a map, which is much easier to maintain and which reads much easier.
move the strings to flash memory to save RAM.

* DPL: use VictronMpptClass::getPowerOutputWatts method

remove redundant calculation of output power from DPL. consider
separation of concern: VictronMpptClass will provide the total solar
output power. the DPL shall not concern itself about how that value is
calculated and it certainly should be unaware about how many MPPT charge
controllers there actually are.

* VE.Direct: avoid shadowing struct member "P"

P was part of the base struct for both MPPT and SmartShunt controller.
however, P was also part of the SmartShunt controller data struct,
shadowing the member in the base struct.

since P has slightly different meaning in MPPT versus SmartShunt, and
since P is calculated for MPPT controllers but read from SmartShunts, P
now lives in both derived structs, but not in the base struct.

* VE.Direct: isDataValid(): avoid copying data structs

pass a const reference to the base class implementation of isDataValid()
rather than a copy of the whole struct.

* VE.Direct: unify logging of text events

* VE.Direct: stop processing text event if handled by base

in case the base class processed a text event, do not try to match it
against values that are only valid in the derived class -- none will
match.

* VE.Direct MPPT: manage data in a shared_ptr

instead of handing out a reference to a struct which is part of a class
instance that may disappear, e.g., on a config change, we now manage the
lifetime of said data structure using a shared_ptr and hand out copies
of that shared_ptr. this makes sure that users have a valid copy of the
data as long as they hold the shared_ptr.

* VE.Direct MPPT: implement getDataAgeMillis()

this works even if millis() wraps around.

* VE.Direct: process frame end event only for valid frames

save a parameters, save a level of indention, save a function call for
invalid frames.
Copy link

github-actions bot commented Apr 4, 2024

This issue 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 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
4 participants