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

P251_PZEM004Tv30_Multiple #145

Closed
wants to merge 20 commits into from
Closed

P251_PZEM004Tv30_Multiple #145

wants to merge 20 commits into from

Conversation

djelau
Copy link

@djelau djelau commented Dec 1, 2019

Current PZEM004T plugin is not compatible with PZEM004Tv30 (modbus communication). It is also not compatible with commmunication with several PZEM004T on the same serial link.
Proposal is a new Plugin for new PZEM004Tv30: Voltage/current/power/energy/frequency/cos phi probe using serial Modbus communication protocol.

Features:

Read values
Erase energy value
Set and reset address
Access to several PZEMs simultaneously

@TD-er
Copy link
Member

TD-er commented Dec 1, 2019

You completely deleted the Readme.md file.
Can you move the readme.md file you created to somewhere else? It is specific to your plugin changes, not the whole repository.

_P251_PZEM004Tv3.ino Outdated Show resolved Hide resolved
@TD-er
Copy link
Member

TD-er commented Dec 2, 2019

Hmm that's not what I meant.
Now you deleted all files.

Do you need help with Git?

@djelau
Copy link
Author

djelau commented Dec 2, 2019

OK, I put back the original readme.md file in my repositery and pull again.
For the other files that I delete, I don't use it. That why I 've kept only .h and .cpp that I use. Is it an error ?
Thanks for your comments. I'll update my ino code soon.

@TD-er
Copy link
Member

TD-er commented Dec 2, 2019

For the other files that I delete, I don't use it. That why I 've kept only .h and .cpp that I use. Is it an error ?

Well it looked like you deleted all files when I made my comment.
We usually keep all files in the lib dir, if it is an external library.
For example the library.properties and library.json file do show what the library is about, what version it is and where you can find it.
That makes maintenance a lot easier.

@djelau
Copy link
Author

djelau commented Dec 2, 2019

It is based on an external library but I modify .cpp/.h to be compatible with espeasyserial and add possibility to change PZEM address dynamically. I'm gonna see how I can clarify my position with the official PZEM library (fork the library ?).
Otherwise, I can also try to integrate the library directly into my pluggin but some compile errors occur...

@TD-er
Copy link
Member

TD-er commented Dec 2, 2019

If you look at other libraries we use, they are copied into the libs folder of the repo and some are modified. (for example pubsubclient)
You can add a note to the library files when it has been modified.
As long as we know the original version it is very easy to see what has been changed.

{
addHtml(F("Tx Pin and Rx Pin have no effect on the configuration as this PZEM is not the main configured."));
{
Device[deviceCount].Type = DEVICE_TYPE_DUMMY; //Erase GPIO choice if not first PZEM
Copy link
Member

Choose a reason for hiding this comment

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

You know the device type is also used to determine the number of output values?
And the Device vector contains the plugin definitions, not the task definitions.
This will have an effect on all instances of the same plugin on an ESP node.

Copy link
Author

Choose a reason for hiding this comment

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

For me, "type" only defines the number of datapin (Vtype for # of output):
#define DEVICE_TYPE_SINGLE 1 // connected through 1 datapin
#define DEVICE_TYPE_DUAL 2 // connected through 2 datapins
#define DEVICE_TYPE_TRIPLE 3 // connected through 3 datapins
Am I wrong ?

Thus how can I remove the data pin selection on other instance of the same pluggin (any example on an existing pluggin)?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, you're right it is about the pins, not the values.
But still, values in that vector are for all instances of the same plugin, not per task.
If you need to spilt it, you can set it to some "no pins" value, but then you must add the pin configuration yourself in the PLUGIN_LOAD (and PLUGIN_SAVE) based on other settings of the task config.
Or you can make 2 versions of the plugin, which do share a common C++ object to communicate with the hardware.
I think you should have a single central object shared among several task instances when communicating over a shared bus.
This is also an issue for the Dallas sensors.

The Eastron plugin does just allow multiple instances as long as you set all to use the same pins. There is a shared object which does the actual communication.
But there you may want multiple task instances to collect different values, since those output way more values than the 4 we can handle on a single task.

@TD-er
Copy link
Member

TD-er commented Dec 19, 2019

Changes look OK. If you are done with it, I can merge it.

I quickly browsed through the code and I kinda like this concept:

if (P251_PZEM_FIRST == event->TaskIndex) //If first PZEM, serial config available
{
int rxPin = CONFIG_PIN1;
int txPin = CONFIG_PIN2;
if (P251_PZEM_sensor != nullptr) {
// Regardless the set pins, the software serial must be deleted.
delete P251_PZEM_sensor;
P251_PZEM_sensor = nullptr;
}
// Hardware serial is RX on 3 and TX on 1
P251_PZEM_sensor = new PZEM004Tv30(rxPin, txPin);

But it has a drawback. If the first instance is removed (e.g. you re-order the tasks), you have to reboot in order to apply settings.
We can use the idea with other plugins that need a central handler for multiple devices, like the Dallas sensors.

@uzi18 what do you think of this idea of letting the first instance handle the init of a central handler?
We can make a generic function for it, so we can use it in several similar plugins.

@uzi18
Copy link

uzi18 commented Dec 19, 2019

@TD-er simple if (P251_PZEM_sensor == nullptr) P251_PZEM_sensor = new PZEM004Tv30(rxPin, txPin); should be enaugth but what if one task has got different rx/tx configuration than another?

how to be sure if (P251_PZEM_FIRST == event->TaskIndex) is first one?
core should copy this specific (rx/tx) configuration to next task used by this device

by the way I need something like first/last/count tasks for specific device for 1wire, not sure if it is already implemented

@TD-er
Copy link
Member

TD-er commented Dec 19, 2019

by the way I need something like first/last/count tasks for specific device for 1wire, not sure if it is already implemented

Is not yet implemented.
But can't you use some kind of smart pointer to the object for it?
If the last one deletes the pointer, the object will be destructed.

But that discussion can be continued on the Dallas issue I guess :)

@djelau
Copy link
Author

djelau commented Dec 19, 2019

I didn't have time to give some comments to my changes that you already review my release. Thank for your reactivity.
I'm aware with the limitation in case of deletion of the first instance. I don't imagine simple method to overcome this limitation: big effort (and code) for a very few gain. But I forget to check recently what happens after deletion.
And in addition, as HMI is very user-friendly (thanks to your job !), if first instance is removed, it is easy to add it again.

@djelau
Copy link
Author

djelau commented Dec 19, 2019

@TD-er simple if (P251_PZEM_sensor == nullptr) P251_PZEM_sensor = new PZEM004Tv30(rxPin, >txPin); should be enaugth but what if one task has got different rx/tx configuration than another?

Not very optimised to use different serial interface to communicate with several PZEM when only single serial is sufficient. Don't you think ?

how to be sure if (P251_PZEM_FIRST == event->TaskIndex) is first one?
core should copy this specific (rx/tx) configuration to next task used by this device

Prior this test, I allocate:
if (P251_PZEM_sensor==nullptr) P251_PZEM_FIRST = event->TaskIndex; //To detect if first PZEM or not
Thus I know what is the index of the first instance. Is it enought from your point of view ?

@TD-er
Copy link
Member

TD-er commented Dec 19, 2019

You only set it at the init of this plugin with the lowest index.
But if you delete or disable that one, it will never be set again.

I think we should add a central mechanism for that, so you don't have to fix it here.

@uzi18
Copy link

uzi18 commented Dec 19, 2019

@djelau
Ok you are right, about first index
When you add 2 tasks and remove first one there will be no communication configuration when you reboot esp and to configure it one more time you will need to add another PZEM004T task

There is solution, just use struct with pointer to P251_PZEM_sensor, rx pin and tx pin number, first task number.
Then in webform save set it from struct, like:

        CONFIG_PIN1  = my_global_struct.rx_pin;
        CONFIG_PIN2  = my_global_struct.tx_pin;

to set it for every task, but you still need to set :
my_global_struct.first_task = task_find_first_device(PLUGIN_ID_251); //just idea ;)

@TD-er
I mean it is nice to have some functions reusable by other plugins.

@TD-er
Copy link
Member

TD-er commented Dec 19, 2019

I mean it is nice to have some functions reusable by other plugins.

Couldn't agree more!
You know about my allergy.

@freeesty
Copy link

used this plugin for 6 month with esp32 mega build. But since ESPEasySerial 2.0.4 its incompatible.

@TD-er
Copy link
Member

TD-er commented Oct 24, 2020

used this plugin for 6 month with esp32 mega build. But since ESPEasySerial 2.0.4 its incompatible.

I will make a fix for it, as it is quite a simple fix.
With the latest ESPEasySerial, you can now select the port + GPIO pins on ESP32.

@TD-er
Copy link
Member

TD-er commented Oct 24, 2020

Can you test with this PR?
letscontrolit/ESPEasy#3337

I've not yet added it to a build config, but I guess you could build it yourself?

Here the custom builds I made to test whether it would compile.

@TD-er
Copy link
Member

TD-er commented Oct 24, 2020

Hmmm just noticed the plugin did not set the Type to be DEVICE_TYPE_SERIAL so it will not show the right serial selector in the web page.
This may need some more work...

@freeesty
Copy link

yeah, with the my custom build I used HW1 with gpio pins 13 and 15.
With the new version I cant choose pins.

@TD-er
Copy link
Member

TD-er commented Oct 25, 2020

Yep, the P251 version did not really use the ESPEasySerial wrappers as intended.
Will have a look at it tomorrow... or to be more precise, after some sleep.
It's already past 2am here.

With David Gilmore on Youtube, headset on and nerd mode active, one may forget the time :)

@freeesty
Copy link

:) same timezone here. But one more hour of sleep this night. And thanks for your help & work! Good night

@TD-er
Copy link
Member

TD-er commented Oct 25, 2020

:) same timezone here. But one more hour of sleep this night. And thanks for your help & work! Good night

Well, as it is becoming winter time, you will have an extra hour, so if you wait a few minutes I may have a new test build for you

@TD-er
Copy link
Member

TD-er commented Oct 25, 2020

A new test build

Screenshot on ESP8266:
image

On ESP32 it should show something like this: (taken from some ESP32 node, not running this plugin, just showing serial config how it should look like)

image

@freeesty
Copy link

building P102 with the other test plugins went fluenty.
After flashing I had some missconfiguration which results in core panic and an erase flash of esp32.

Now with uploaded config from backup everything is fine again.
Screenshot_20201025-025855_Chrome

@TD-er
Copy link
Member

TD-er commented Oct 25, 2020

Hmm the GPIO-15 warning should not be shown on ESP32, but still that pin has some limitations.
So good to know that's something I also have to look at.

@TD-er
Copy link
Member

TD-er commented Oct 25, 2020

I'm now looking at this plugin a bit more.
I notice the PZEM modbus address is dealt in the PLUGIN_WEBFORM_LOAD call.
I would expect it to be related to the serial settings as is also done for some other plugins like the AcuDC243.
There does seem to be a special case here where the address needs to be reset and/or where only a single PZEM is connected to the modbus.

Why is the config split here to only allow configuration of the serial settings from the first task running this plugin, combined with warnings about using a new serial port for multiple instances?
The idea of Modbus is to allow multiple devices to use the same bus.
So then you need to program the address to a specific unit.
But the configuration as shown here does indicate setting the address may not be permanent?

Can anyone explain this a bit more, as of why this approach was chosen?

@freeesty
Copy link

freeesty commented Dec 7, 2020

unfortunately I'm not the developer of this plugin and cant provide some first hand information. There is a developer manual from the manufacturer:
https://innovatorsguru.com/wp-content/uploads/2019/06/PZEM-004T-V3.0-Datasheet-User-Manual.pdf maybe this is helpful.

On the other hand I'm facing some stability issues on esp32 with the latest mega release (the previous releases aswell). Is there a standard way of how to approach debuging?

@TD-er
Copy link
Member

TD-er commented Dec 8, 2020

As this PR + plugin now has been moved into the main repository (see: letscontrolit/ESPEasy#3337 )
I guess it is a better idea to test out that version first and if it still has issues, please open a new issue on the ESPEasy repository as it is now part of the ESPEasy code. (and should soon be included also in builds in the nightly build)

To avoid confusion, I will close this PR.
Please let me know if I'm missing something.

@TD-er TD-er closed this Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants