TheWandrr / vedirect_to_mqtt

MIT License
8 stars 5 forks source link

Improve idle behavior #7

Open rp- opened 2 years ago

rp- commented 2 years ago

Instead of constantly looping and wasting CPU cycles, sleep or block for input data. Reduces load from 100% to 3% on my raspberry pi zero.

TheWandrr commented 2 years ago

Thanks for your contributions.

In principle I don't like the idea of code that blocks for an arbitrarily long period of time that's noticeable to the user. I agree completely that the idle behavior is horrible though.

I'm not currently set up to test this with hardware, so can you tell me what happens on shutdown or with some edge scenarios with your changes? Is there any case where this appears to "hang" for up to 10 seconds? What about abrupt serial disconnection and re-connection? Does that work gracefully?

Also, why 10 seconds? Does this give better idle performance than say, 2 seconds?

I'd be more inclined to merge a change that didn't include a long delay. Have you considered other ways that the idle behavior could be improved?

rp- commented 2 years ago

Yeah it might be possible that, the thread will block there for 10s, until the serialGetchar returns, but for the vedirect case rather unusual, as you should get data every second.

Another way would be to keep it like the old code was and just add an usleep between serialDataAvail.

But in my tests I didn't see any long shutdown behavior, but I also didn't test any broken serial port connections.

fregger2023 commented 1 year ago

This fixed my Pi3 model B setup. CPU went from 300% to under 1% while still having sufficient mqtt output. Temperature dropped from 65 to 41 degrees. Well done :-)

rp- commented 1 year ago

This fixed my Pi3 model B setup. CPU went from 300% to under 1% while still having sufficient mqtt output. Temperature dropped from 65 to 41 degrees. Well done :-)

FYI: if you are interested in a vedirect -> prometheus bridge (instead of mqtt) you might be interested in this: https://github.com/rp-/vedirect-prom

fregger2023 commented 1 year ago

I do have a remark. I noticed that since this change the process gets killed about every 12-15 seconds, to then be restarted by watchdog/systemd:

23:30:09 vedirect vedirect_to_mqtt[18256]: <<< Publish bmv/hex/main_voltage = 23.28 23:30:09 vedirect systemd[1]: vedirect_to_mqtt.service: Watchdog timeout (limit 15s)! 23:30:09 vedirect systemd[1]: vedirect_to_mqtt.service: Killing process 18256 (vedirect_to_mqt) with signal SIGABRT. 23:30:09 vedirect systemd[1]: vedirect_to_mqtt.service: Main process exited, code=killed, status=6/ABRT 23:30:09 vedirect systemd[1]: vedirect_to_mqtt.service: Failed with result 'watchdog'. 23:30:19 vedirect systemd[1]: vedirect_to_mqtt.service: Scheduled restart job, restart counter is at 2950. 23:30:19 vedirect systemd[1]: Stopped VE.Direct to MQTT Bridge. 23:30:19 vedirect systemd[1]: Started VE.Direct to MQTT Bridge. 23:30:19 vedirect vedirect_to_mqtt[18261]: <<< Publish bmv/hex/main_voltage = 23.27

As a result, every 12-15 seconds there is a 10 seconds delay. I guess this is not 'by design', but I still prefer it over the original that was overcooking all my test-Pi's :-). Unfortunately I don't have any programming skills but I'm happy to test any offered improvements in the code.

TheWandrr commented 1 year ago

I'm not actively working on this, but do review comments and requests from time to time.

Neither the original code nor the change seem like a great solution. I'm going to let this one percolate a while longer and see if there's any other input. At least people have the option to try the solution and see whether they like it better.