mandulaj / PZEM-004T-v30

Arduino library for the Updated PZEM-004T v3.0 Power and Energy meter
MIT License
256 stars 108 forks source link

Can you make this code non-blocking? #116

Open ham3dt opened 7 months ago

ham3dt commented 7 months ago

Is it possible to make this code non-blocking?

Nismonx commented 7 months ago

@ham3dt that's what pull requests are for!?

ham3dt commented 7 months ago

@Nismonx Sorry I'm new to this, can you do it though? is it possible?

Nismonx commented 7 months ago

@ham3dt I think you are missing the point.

Using chatgpt to question a devs work and asking him to put it right not even knowing if your source is correct.

And no, I can't do it. It's working fine for me.

Is there a reason for this request? In othere words, are you expecting an issue that made you look in to the solution?

ham3dt commented 7 months ago

@Nismonx Please don't get me wrong, I'm not questioning your work and I appreciate the library you provided for free, thank you.

There's another library which uses non-blocking code: https://github.com/vortigont/pzem-edl

I was wondering if you can please do a few changes to make this library non-blocking as well.

I also edited the request content, sorry if it was offending.

Nismonx commented 7 months ago

@ham3dt , for the record this is not my work.....

But you getting there now.

Over to mandulaj.

ham3dt commented 7 months ago

@Nismonx I thought you're at least a contributor 😄

@mandulaj Pretty please?👉👈

sergiomnb commented 7 months ago

hi all. @Nismonx @ham3dt that's what pull requests are for!? I don't think this is the way to behave ,at least it wasn't once ,maybe habits have changed.

@ham3dt Is it possible to make this code non-blocking?

it would be interesting to understand what problems you have with the library with the non-blocking code and why you don't use that. If it was because it only works with the ESP32 while you want to use it with another device I could tell you that no ,this one to use other devices fails to implement a type of real time communication as you can do with the ESP32. Otherwise better clarify your goals,the devices you are using,the code you are using and why you need the non-blocking code so that we can help you.

Sergio

ham3dt commented 7 months ago

@sergiomnb How did I behaved? you like making drama out of nothing?

Yes, that library is for ESP32 and I'm using this of course for Arduino. I'm planning to do some math and averaging and TBH this library is a bit slow.

So I appreciate it if you can do some optimizations to the code, as I'm reading PZEM004Tv30.cpp file there are a lots of "to do" that developers commented such as:

PZEM004Tv30::~PZEM004Tv30()
{
    // TODO: Remove local SW serial
    // This is not the correct way to do it. 
    // Best solution would be to completely remove local SW serial instance and not deal with it.
#if defined(PZEM004_SOFTSERIAL)
    if(this->localSWserial != nullptr){
        delete this->localSWserial;
    }
#endif
}
mandulaj commented 7 months ago

Hello all, calme vous guys! Let's keep this professional.

To @ham3dt request:

Is it possible to make this code non-blocking?

Yes, in theory, everything is possible. But at what cost?

This library barely supports both basic Arduinos as well as ESP8266 and ESP32. This means any time you want to make a change, you have to verify it on all platforms.

I would be tremendously happy to see a DMA based asynchronous serial read, but, that would require quite significant rework. If you find the time, you are welcome to have a look into this and implement a fully asynchronous receive. However good luck making it cross-platform.

Is this a lazy answer? Yes, but the fact is, I don't have a lot of free time to implement this functionality. And if the current implementation does the job well 90% of the cases, I won't invest my time to cover the 10% edge case scenarios.

More importantly, I need to see clear arguments, why you can't get away with using the synchronous read.

And as @sergiomnb already said, I have a feeling that this is a XY problem. What exactly is the issue that you are running into besides ("the library being slow"). Perhaps there is an alternative solution to your problem to what ChatGPT suggested.

X

sergiomnb commented 7 months ago

@ham3dt

@sergiomnb How did I behaved? you like making drama out of nothing?

It wasn't meant for you.