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

Pulse Interfacer #105

Merged
merged 11 commits into from Sep 21, 2020
Merged

Pulse Interfacer #105

merged 11 commits into from Sep 21, 2020

Conversation

borpin
Copy link
Contributor

@borpin borpin commented Jun 11, 2020

Create a Pulse Interfacer to read an optical led pulse sensor directly with an RPi.

Documentation will be updated once the MQTT interfaces changes are merged.

@borpin borpin marked this pull request as ready for review June 11, 2020 17:06
@TrystanLea
Copy link
Member

Thanks @borpin, looks like a nice interfacer! testing here I has an error due to missing RPi module:

python3 src/emonhub.py --config-file=/etc/emonhub/emonhub.conf
Traceback (most recent call last):
  File "src/emonhub.py", line 26, in <module>
    from interfacers import *
  File "/opt/openenergymonitor/emonhub/src/interfacers/EmonHubPulseCounterInterfacer.py", line 5, in <module>
    import RPi.GPIO as GPIO
ModuleNotFoundError: No module named 'RPi'

Looking for a solution suggests checking for the module before importing it e.g:
https://stackoverflow.com/questions/14050281/how-to-check-if-a-python-module-exists-without-importing-it

import importlib
RPi_spec = importlib.util.find_spec("RPi")
RPi_found = RPi_spec is not None

if RPi_found:
    import RPi.GPIO as GPIO

and then maybe lower down:

        if RPi_found:
            self.init_gpio()

That gets rid of the error for me, but I haven’t been able to test it further than that.

@borpin
Copy link
Contributor Author

borpin commented Jun 12, 2020

@TrystanLea When you added the GPIO library, did you then need to add the emonhub user to the gpio group?

@borpin
Copy link
Contributor Author

borpin commented Jun 12, 2020

I went back to @pb66 original and he used a different method to check - decided to use that method.

Added extra bits to the install. I realised I had installed gpiozero by mistake. Need to test.

@borpin
Copy link
Contributor Author

borpin commented Jun 12, 2020

Switched GPIO install to pip3.

I'll start from scratch again later just to check it installs correctly.

@borpin
Copy link
Contributor Author

borpin commented Jun 29, 2020

Note community topic https://community.openenergymonitor.org/t/maximum-number-of-pulses/14655/4

The pulse interfacer reports on every pulse. This needs to be rate limited as at high loads it can overwhelm emonhub and result in missed pulses.

@bwduncan any suggestions on how best to implement a rate limit?

Perhaps a parameter in seconds to define the rate (gap)?

If last + rate < now() exit else continue.

Is that efficient?

@bwduncan
Copy link
Contributor

The first thing I would look at is the interrupt callback. It should run as fast as possible. Right now it has logging in it which is orders of magnitude slower than the dict update we actually need.

Can you try it again without the log line in process_pulse?

@bwduncan
Copy link
Contributor

Basically I'm thinking that calls to read will be rate limited by the 200ms sleep anyway, so if we can't get everything done in that time then it's never going to work.

@borpin
Copy link
Contributor Author

borpin commented Jun 29, 2020

But you could let the callback just increment the counter without actually outputting the value. The read simply gets a value to return, if it returns nothing then the MQTT process does not get triggered for instance.

@bwduncan
Copy link
Contributor

The problem is the impedance mismatch. It's impossible for us (the pulse interfacer) to know how fast to provide data, we can only do so when it is asked. Any arbitrary delay there is just that. The emonhub main loop knows how much work it has to do and will only call us as fast as it can, with its 200ms delay. Everything there is happening serially, trying to drop work out at the leaves like this is the wrong approach.

Unless the machine is seriously overloaded, it's not really possible for this mechanism to miss pulses, unless we are doing too much work in the interrupt callback.

Was there an improvement with the logging removed from process_pulse?

@bwduncan
Copy link
Contributor

Thinking about it more, the logging is probably one of the the worst things we could be doing, since it will force python to context switch to the wrong thread, waiting for whatever was happening there to finish, and then back to the interrupt thread.

@borpin
Copy link
Contributor Author

borpin commented Jun 30, 2020

Pushed rate limited changes as discussed. passed as a parameter so can be configured.

@bwduncan
Copy link
Contributor

Not really discussed, but your implementation seems pretty close to what other interfacers are doing in the absence of a higher-level policy.

I really was interested in seeing whether the logging was responsible for gumming up the works...

@borpin
Copy link
Contributor Author

borpin commented Jun 30, 2020

@TrystanLea @bwduncan - firstly, if I have this wrong please tell me!!

It occurs to me that if the time now was stored when the pulse was read a 'power now' figure could be calculated and returned with the cumulative count.

You just need to store the time of the last pulse of the previous frame and the time of the most recent pulse in this frame and then calculate the power used in W for the period.

You could pass in the number of pulses per Wh as a configurable item.

Would that work?

@borpin
Copy link
Contributor Author

borpin commented Jun 30, 2020

I really was interested in seeing whether the logging was responsible for gumming up the works...

It will be to an extent, but it is the MQTT/HTTP processing that takes the time on each emonhub iteration especially if you send each value as a separate topic. I've helped that by modifying the mqtt interfacer to send a JSON object, so a single publish per frame, but that has not been merged yet (nudge nudge @TrystanLea #101 😀 ). My problems here were exacerbated by me sending it to 3 different brokers 😆.

Even without the logging, you will need to rate-limit the times read returns a value.

I will remove the callback log for production though just to be sure.

@bwduncan
Copy link
Contributor

I really was interested in seeing whether the logging was responsible for gumming up the works...

It will be to an extent, but it is the MQTT/HTTP processing that takes the time on each emonhub iteration especially if you send each value as a separate topic. I've helped that by modifying the mqtt interfacer to send a JSON object, so a single publish per frame, but that has not been merged yet (nudge nudge @TrystanLea #101 😀 ). My problems here were exacerbated by me sending it to 3 different brokers 😆.

I must not be understanding this architecture properly. Even if the MQTT/HTTP interfacers are sending data at a leisurely pace, the callback will still count pulses in its own thread as fast as they arrive (unless of course it has to wait for the logging to finish...). The linux kernel scheduling latency shouldn't be more than 6ms, or 166Hz, so we would have to seriously starve the thread for it to miss pulses.

Even without the logging, you will need to rate-limit the times read returns a value.

  1. Why? There's already a 200ms sleep upstream. read can never run faster than 5Hz.

  2. That rate-limiting should be done higher up, not by us. We have no business knowing what frequency we should be sending data. In principle, each subscriber should be able to say how quickly it can process the data. For example the emoncms interfacer knows that it will send data once every 30 seconds (or at least that's the value specified in my emonhub.conf.

I will remove the callback log for production though just to be sure.

Thanks! 😄

I would love to try some measurements. Would you mind seeing whether this still happens if you put a time.sleep(1) (or longer) in one of the other interfacers? The MQTT one would be a good target, to see whether this problem is actually caused by sending too much data through it... I think you should still see the pulses being counted correctly, even if the main thread is blocked. It would also be interesting to see what happens if you time.sleep(1) the pulse counter read.

@borpin
Copy link
Contributor Author

borpin commented Jul 1, 2020

I'm not sure of the overall mechanism but it is not just about the callback not firing, it is the whole process. emonhub is not designed to send data out via MQTT or HTTP at the sort of rate that the pulse sensor can send it at - the interfacer needs to accumulate the pulses and send that number on at a lower rate - that is what is now being limited. Without that, emonhub is trying to send data on every 200ms; it will not have even finished the previous MQTT/HTTP tasks in that time.

AIUI

  • emonhub polls each interfacer for 'have you anything to do', does that, then sleeps for 200ms, before polling again.
  • In the background certain callbacks, such as serial and pulse tasks can run and load a data buffer.
  • when emonhub polls those interfacers, they will either respond with 'here is my data to deal with', or 'nothing to do here'.
  • As it stood, the pulse interfacer responded every 200ms with, yep send this data on. This is what is being limited.

I doubt that if you started from scratch you would design it that way now!

The 200ms sleep was introduced as it was discovered that emonhub was taking all the CPU, as it just went round and round the poll without stopping - it is a bit crude!

emonhub still uses a lot of CPU for a relatively simple task.

@danbates2
Copy link
Contributor

danbates2 commented Jul 2, 2020

Hi, a couple of minor notes from testing this yesterday.
. I had to run install.sh to add the user emonhub to pymodule rpi.gpio. Perhaps this could be something checked for when emonhub restarts – with an appropriate error in the log.
. An argument exists to have rpi.gpio or python3-gpiozero installed by default on emonSD, or through the emonScript install script.
. Changing bouncetime and rate_limit required me to restart emonhub for pulses to be sent forward to emoncms, doing this from the web gui was sufficient, otherwise the log would show 'deleting interfacer...' and not indicate restarting the interfacer.

Everything else checked out fine – bouncetime, rate_limit, and pulse counting at a speed of 40ms per pulse, with posts to emoncms happening during these fast pulses, all worked as expected and all pulses were caught.

@borpin
Copy link
Contributor Author

borpin commented Jul 2, 2020

. Changing bouncetime and rate_limit required me to restart emonhub for pulses to be sent forward to emoncms, doing this from the web gui was sufficient, otherwise the log would show 'deleting interfacer...' and not indicate restarting the interfacer.

Because they are not a runtime settings you do need to restart emonhub to change it.

I had to run install.sh to add the user emonhub to pymodule rpi.gpio

Yes. Probably needs adding to an update script.

@danbates2
Copy link
Contributor

updated the above comment to include a mention of python3-gpiozero, which @borpin has just introduced me to.
. is it correct that python3-gpiozero provides the modules for the interfacer to work? I installed rpi.gpio instead, which I think did the trick.
. I think rpi.gpio provides what is necessary in less disk space – it might be better for that reason.

@borpin
Copy link
Contributor Author

borpin commented Jul 2, 2020

@bwduncan @danbates2

Which is better?

sudo apt-get install rpi.gpio
sudo apt-get install python3-gpiozero

@danbates2
Copy link
Contributor

Which is better?

I've had a check of both, neither I'd say.

gpiozero includes rpi.gpio, plus a few other (potentially useful) things.

The following NEW packages will be installed:
  python3-colorzero python3-gpiozero python3-rpi.gpio python3-spidev rpi.gpio-common
0 upgraded, 5 newly installed, 0 to remove and 141 not upgraded.
Need to get 128 kB/154 kB of archives.
After this operation, 836 kB of additional disk space will be used.

might as well install gpiozero for python3.

@danbates2
Copy link
Contributor

Yes. Probably needs adding to an update script.

@TrystanLea a quick note to make sure you're on the same page – it seems be a good idea to 1. sudo apt-get install python3-gpiozero and 2. add user emonhub to the gpio thingy.
The detail of the latter seems already included in emonhub install.sh, which could be copied into the update script.

@bwduncan
Copy link
Contributor

bwduncan commented Jul 2, 2020

We're not using gpiozero in emonhub. Only python3-rpi.gpio is required.

However, someone decided to use gpiozero in the LCD display script, so it is needed there. I don't know what the overlap is, but I suspect it will get installed anyway.

It's not correct but it won't do any harm to just install python3-gpiozero for emonhub.

@bwduncan
Copy link
Contributor

bwduncan commented Jul 2, 2020

rate_limit could be a runtime parameter which can be updated on the fly, but aside from debugging I'm not sure that would ever be useful...

@borpin
Copy link
Contributor Author

borpin commented Jul 2, 2020

rate_limit could be a runtime parameter which can be updated on the fly, but aside from debugging I'm not sure that would ever be useful...

You are right, but couldn't be bothered to fiddle with the settings bit 🤣

@danbates2
Copy link
Contributor

@borpin Does the pulse event write to a log file on disk? I see it's at log level DEBUG, but I don't really know how emonhub logging works.

@borpin
Copy link
Contributor Author

borpin commented Jul 16, 2020

Does the pulse event write to a log file on disk?

Yes it writes to the emonhub log.

@danbates2
Copy link
Contributor

It crossed my mind pulses can happen very often, so could hog up the log file, and cause excessive writes or a larger than necessary log file. Cause for concern?

@bwduncan
Copy link
Contributor

It's worse than that, if the pulse thread is busy logging when the next pulse comes in, it will miss pulses altogether. The logging has been removed from that thread, but there is still (harmless) debug logging in the thread which collects the pulses and passes them on.

@borpin
Copy link
Contributor Author

borpin commented Jul 17, 2020

It crossed my mind pulses can happen very often, so could hog up the log file, and cause excessive writes or a larger than necessary log file.

As Bruce says, bigger concern is missing a pulse. Taking out line 87 when it goes live will need to be done. It doesn't seem to be an issue although the more pulses per kWh the meter makes the more it may be an issue.

emonhub, by default, rotates it's log itself at 500MB, so this shouldn't be an issue.

However, some of the logging could be removed as it was me checking things were happening as expected.

@danbates2
Copy link
Contributor

I see.
If there's a print() clearly that won't log by default. I guess there's a way to listen to a process' print out regardless of logging?

@TrystanLea TrystanLea merged commit 8c90835 into openenergymonitor:master Sep 21, 2020
@TrystanLea
Copy link
Member

Thanks @borpin sorry for the delay in merging this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants