Pulse Counter P003 enhancement. How to give back?

Moderators: grovkillen, Stuntteam, TD-er

Post Reply
Message
Author
Kim_Panda
Normal user
Posts: 11
Joined: 25 Jan 2021, 17:03

Pulse Counter P003 enhancement. How to give back?

#1 Post by Kim_Panda » 29 Jan 2021, 16:10

Hello Community,
I developed an enhancement to stabilize the Generic Pulse Counter P003 with two further Mode Types. I want to offer that enhancement as give-back to the community but have no experience on doing it in a community forum like this. After spending a lot of time in that, I ran out of time and I hope that some experienced user takes my work and moves it over into the ESPEasy development branch. PLEASE TAKE OVER
In addition I found a bug/problem in the current "LOW" Mode Type and I suggest a correction (see below).

Here are the details:
I was looking for an ESP8266 firmware that counts my reed switch pulses coming from my gas meter and transfers it via MQTT to ioBroker. I found ESPEASY, a solution that I appreciate (extremely good work, well structured and clear coding! Thank you!) and I set up the environment and tested with the available option of the Generic Pulse counter.
But all did not correctly count the reed switch pulses. I spent a lot of time with forum discussions and solutions with the ESPEasy solution, finding more and more people who gave up, tried various hardware options or lived with solutions that not count really correctly.
Thus, still being convinced of the ESPEasy approach, I decided to enhance the Generic Pulse Counter P003. I found a solution which suits my purpose very well and which very stable counts my reed switch pulses coming from my gas meter.
And now is the time where I want to give my work back to the community, BUT I have absolutely no experience with forums and development communities like this and I do not know how to give back.
I want somebody who does that for me, because I am running out of time.

What I am offering:
§ The modified Device Plugin "_P003_Pulse.ino" (see attachment) form Version "ESPEasy-mega-20210114" with change markers in the code and a lot of comments, that explain the new functionality and the changes. The effectively changed and added lines of code is totally only a 23 lines.
§ I build it on the current Visual Studio Code / PlatformIO in the environment "custom_ESP8266_1M" tailored down in "pre_custom_esp82xx.py" to only
"-DUSES_C005", <-- Home Assistant (openHAB) MQTT
"-DUSES_P003", <-- Generic – Pulse counter
"-DFEATURE_MDNS".
§ I successfully debugged and tested it on a tiny ESP8266/ESP-01 with the simplest hardware consisting only of a simple reed switch between GPIO 2 and GND (and Power Supply).
§ The description of the new functionality (see below).
§ And not to forget: A suggested small correction for a basic problem with Mode Type "LOW" in the current solution.

Analysis and Description of Enahancement:
I was not satisfied with the functionality of the Device-Plugin Pulse Counter ( _P003_Pulse.ino) for counting reed switch pulses from my gas meter. So I did some analysis and then came to the following:
The current implementation determines the debounce time between counted pulses. This means, that too frequent consecutive pulses are ignored. This may make sense for certain applications, e.g. to detect pikes, but is not appropriate if you want to count impulse blocks e.g. of power or gas meters which are detected by magnetic contacts (reed switch).
Although the current "CHANGE" mode sounds appropriate for reed switch counters if you divide the counts by 2, it gets more worse, because through the current debounce behaviour, you never know if you missed an even or uneven number of pulses in the debounce time and the resulting count is not accurate.
Thus I decided to implement an enhanced "CHANGE" mode combined with a modified debounce measurement. Here are the key points:
§ Two new Mode Types are introduced "PULSE low" and "PULSE high".
"PULSE low" intends to count "low" pulses on a GPIO which normally (in its inactive state) is high.
"PULSE high" does it vice versa. It is assumed that the active pulses are shorter than the duration of inactive phases.
§ This allows to easily define the Debounce Time on that active pulse. As a rule you can say, that the Debounce Time should be between ½ of the expected minimal duration of a stable active pulse. This provides secure determination of expected pulses and eliminates all shorter debounce effects, both in the active and the inactive state!
In summary: Active and inactive pulse periods are correctly determined as long as they are not shorter than the debounce time and the active ones are counted.
§ The delivered "Time" (PulseTime) is now the duration of the most recent detected active pulse, which provides for more sophisticated analysis further down the line. E.g. "How long was the contact closed?", rather than "since when was no pulse counted?" as for the other Mode Types.
§ It should be noted, that this mechanism requires counting of active pulses at their end. We need their duration, in order to decide if they are valid or not. This is also the reason, why we distinguish between "PULSE low" and "PULSE high". I.e. counters are incremented when the selected pulse type ends.

Notes:
# I implemented the required changes in _P003_Pulse.ino in version "ESPEasy-mega-20210114".
# Code changes are marked by "// Mx: 2021-01:".
# There are a lot comments added to document the new functionality.
# The implementation is compatible to original version.
# In addition I adjusted the naming of the first Mode Type according to the detected problem as described below.

Detected Problem with Mode Type "LOW" in _P003_Pulse.ino
The interrupt on "LOW" is not correctly implemented, because LOW = 0 ( "modeValues[0] = LOW;" ) is assigned in case "case PLUGIN_WEBFORM_LOAD" for p003_raisetype.
Correct would be ONLOW = 0x04 (cf. Arduino.h) to trigger on low signal.
The current LOW=0 in the ESP8266 GPIO Interrupt Register actually disables the interrupt.
However, using the correct ONLOW=0x04 leads to another problem: When for ONLOW instead of LOW in the Device dialog for p003_raisetype the following happens:
The Interrupt is triggered now and Plugin_003_pulsecheck correctly increments the Counter, BUT only if the GPIO is very shortly pulled down to ground (low). When keeping it low for a while, I see that the logging stucks and after a few seconds the ESP8266 performs an automatic restart. I guess the reason is, that during the GPIO is held down, one interrupt after each other is fired and blocks the whole processing.
I guess, that this problem is the reason, why the opposite Mode Type "HIGH" is also not implemented here.
I recon that a correct implementation needs basic redesign and I doubt that it makes really sense for the Pulse Counter.
So my pragmatic suggestion is, to rename modeRaise[0] = F("LOW"); to "none" and replace LOW by 0 in modeValues[0] = LOW; or simply remove the LOW option completely, as it makes no sense. But this could cause problems with downwards compatibility. So better simply rename to "none".
Attachments
_P003_Pulse.zip
_P003_Pulse.ino - with changes
(3.84 KiB) Downloaded 21 times
How it looks in the dialog
How it looks in the dialog
New_P003_ Device-Task-Settings_dialog.jpg (52.02 KiB) Viewed 1190 times

GravityRZ
Normal user
Posts: 155
Joined: 23 Dec 2019, 21:24

Re: Pulse Counter P003 enhancement. How to give back?

#2 Post by GravityRZ » 29 Jan 2021, 16:45

interesting.
The modified pulse low and pulse high might be a good implementation as long as the debounce time really blocks every pulse which is faster than the debouncetime.(so not once but always) and does so acoordingly to the type you have set(pulse high or pulse low)

regarding the time modification
i see that you just modified it instead of making a second option.
I do not like this implementation because at the moment a lot of people use the time between pulses to calculate eg a waterflow
if it now only times the duration of the pulse(eg the high or the low part) you basically killed the watermeter functionality

i am not experienced enough to implement this but i know there are people on the forum and github who are.

like i said, extra options are always welcome but to make it suitable for everybody we need total pulse time, high time and low time.
i actually made a rules script to to calculate both the high time and the low time to detect jitter on a watermeter.
problem with watermeters are they rotate a disc so it could be that when it detects a fast high or low it is no jitter but the disc was just very close to the sensor.
at the moment i abandoned investigating what is going on in the ESP but now i am measuring the pulses when they arrive in domoticz.
real easy in domoticz to measure on a millisecond level.
Last edited by GravityRZ on 29 Jan 2021, 16:54, edited 1 time in total.

TD-er
Core team member
Posts: 4272
Joined: 01 Sep 2017, 22:13
Location: the Netherlands
Contact:

Re: Pulse Counter P003 enhancement. How to give back?

#3 Post by TD-er » 29 Jan 2021, 16:48

I created a pull request for it with your code changes.
See: https://github.com/letscontrolit/ESPEasy/pull/3482

I will have a good look at it later this evening.

To help you understand what is needed for submitting future pull requests (PR) :)

- Fork the project on Github to your own account.
- Use Git on your PC to checkout your fork of the ESPEasy repository
- Create a branch on the top (HEAD) of the current mega branch with a descriptive name.
- Start hacking
- Create a commit in Git including your changes and give it a descriptive name.
- push your commit(s) to your account.
- Navigate to the ESPEasy repository on Github, to the "Pull requests" tab
- You will see a notification in orange/yellow suggesting you can create a pull request with the new branch you just pushed to your fork.

The idea of a pull request is that you request to merge your branch to the main repository.
So if you later decide to implement a new feature, you should create a new branch (from the mega branch) and repeat the steps above.
If you will continue on the current pull request which you made, you can checkout that branch again and start hacking + commit etc.
As soon as you push your commits, they will be appended to the existing pull request.

GravityRZ
Normal user
Posts: 155
Joined: 23 Dec 2019, 21:24

Re: Pulse Counter P003 enhancement. How to give back?

#4 Post by GravityRZ » 29 Jan 2021, 16:56

@TD-er, can you have a close look at the new Time implementation.

if this changes from total time between pulses to the time a pulse are high it will render the complete watermeter flow functionality useless

TD-er
Core team member
Posts: 4272
Joined: 01 Sep 2017, 22:13
Location: the Netherlands
Contact:

Re: Pulse Counter P003 enhancement. How to give back?

#5 Post by TD-er » 29 Jan 2021, 17:05

GravityRZ wrote: 29 Jan 2021, 16:56 @TD-er, can you have a close look at the new Time implementation.

if this changes from total time between pulses to the time a pulse are high it will render the complete watermeter flow functionality useless
That's one of the reasons I created a PR for it, to show how the review process usually is done on pull requests :)

Kim_Panda
Normal user
Posts: 11
Joined: 25 Jan 2021, 17:03

Re: Pulse Counter P003 enhancement. How to give back?

#6 Post by Kim_Panda » 29 Jan 2021, 17:22

@GravityRZ:
regarding the time modification
i see that you just modified it instead of making a second option.
I do not like this implementation because at the moment a lot of people use the time between pulses to calculate eg a waterflow
if it now only times the duration of the pulse(eg the high or the low part) you basically killed the watermeter functionality
Your assumption is not correct. I may have described it not precicely enough.
The specific new measuring of "Time" only applies to the new modes.
The current modes are left unchanged. So no worries.

The only change to existing functionality I suggested is, to rename the existing "LOW" to "none" in the dialog, as it does not make any sense to me (does nothing). If someone is successfully using "LOW" it would supprise me very much.

Kim_Panda
Normal user
Posts: 11
Joined: 25 Jan 2021, 17:03

Re: Pulse Counter P003 enhancement. How to give back?

#7 Post by Kim_Panda » 29 Jan 2021, 17:38

TD-er wrote: 29 Jan 2021, 16:48 I created a pull request for it with your code changes.
See: https://github.com/letscontrolit/ESPEasy/pull/3482

I will have a good look at it later this evening.

To help you understand what is needed for submitting future pull requests (PR) :)
......
@TD-er:
Thanks for your positive positive responce.
I feel, that it will take me too much time doing it the proper way, because it would be first time for me. I did not use these tools before and I gess it will take me hours or days. For someone experienced it may be only minutes. Therefore I asked for take over.
Next time (when I have more spare time), I will try it properly with your checklist.
Thank you.
Regards

Kim_Panda
Normal user
Posts: 11
Joined: 25 Jan 2021, 17:03

Re: Pulse Counter P003 enhancement. How to give back?

#8 Post by Kim_Panda » 06 Feb 2021, 13:55

TD-er wrote: 29 Jan 2021, 16:48 I created a pull request for it with your code changes.
See: https://github.com/letscontrolit/ESPEasy/pull/3482

I will have a good look at it later this evening.

To help you understand what is needed for submitting future pull requests (PR) :)
......
@TD-er:
I have the enhancement now more than a week in use and must note, that it is not yet as perfect as I want it to be
From 25.000 pulses 95 (0,4%) were not counted. So I will do some further degugging to find out the reason.
There seems something happening behind the scenes that I didn't see yet.

Furthermore GravityRZ's cocerns about the currend "Time" parameter let me think about the semantical change of the parameter "Time" in my new Mode Types. So I think, for compatibility reasons, I will set it also for the new modes to the time, since the last counted pulse. However I still think the PulseLength is a usefull parameter, because it allows adjusting the debounce time to a sensible value.
:?: What do you think? Is is a good idea to add a new parameter "Length" to Pulse Counter P003? And if yes, should it appear only, when one of the new modes is selected or also in the current modes, where it could be set equals to "Time"?

So I will invest some more time in both issues.

TD-er
Core team member
Posts: 4272
Joined: 01 Sep 2017, 22:13
Location: the Netherlands
Contact:

Re: Pulse Counter P003 enhancement. How to give back?

#9 Post by TD-er » 07 Feb 2021, 00:19

Hmm, maybe these things can be better discussed on the Github issue, for a very simple reason...
There I can mark a post as unread, so I can give it the attention it needs when being more sharp in the morning.

GravityRZ
Normal user
Posts: 155
Joined: 23 Dec 2019, 21:24

Re: Pulse Counter P003 enhancement. How to give back?

#10 Post by GravityRZ » 18 Feb 2021, 10:32

as mentioned earlier is am trying to solve a problem regarding extra pulses for months.
I am now focussing on a solution instead of the problem with good results.

Because the debounce time is not perfect i am taking a second step and solved the problem in rules

this is a solution for extra pulses and maybe also for missing pulses because i take action on GPIO level instead of pulsecounter level

the results sofar are very good.

in the last couple of days i have had serveral instances where there was an extra pulse and/or very low time (1ms)
because i am using rules now in combination with a dummy device my dummy counter is still spot on(the dummy is right, the pulsecounter is wrong)

another solution i tried(but did not solve my problem) was the following and will give you exactly what you need

when you use the first pulsecounter and let it change a second GPIO port and use that port for a second pulsecounter you can time exactly when the signal is high, low and how long a high pulse and a low pulse will last.
there is no ms timing in the ESP but there is in the pulsecounter.
Pretty smart that i thought this up and it clearly shows the power of the espeasy software(thanks TD-er)
i have the rules if you have no idea what i am talking about.
this however will not block false pulses/jitter but can be used for other things

watermeter-pulsecounter.JPG
watermeter-pulsecounter.JPG (67.9 KiB) Viewed 527 times

TD-er
Core team member
Posts: 4272
Joined: 01 Sep 2017, 22:13
Location: the Netherlands
Contact:

Re: Pulse Counter P003 enhancement. How to give back?

#11 Post by TD-er » 18 Feb 2021, 10:38

Maybe good to post the rules also, as I don't think I get it what you're doing here.

GravityRZ
Normal user
Posts: 155
Joined: 23 Dec 2019, 21:24

Re: Pulse Counter P003 enhancement. How to give back?

#12 Post by GravityRZ » 18 Feb 2021, 12:41

i do not have a screenshot of the devices but if you read the code you can figur out what devices are configured en what the variables names are.
coding ofcourse could be smoother but i was trying to solve a problem so cosmetics were not my first priority.
i deleted the non involved sections
device 1 pulsecounter called Watermeter hooked up to GPIO14
device 3 MQTT import device filed 1 for MQTT import but using other fields as variables
device 4 i think second pulsecounter
device 5 dummy device with the timing variables

Code: Select all

New version which is using second pulsecounter to detect shortest pulse either low or high
***************************************************

On System#Boot do     						// When the ESP boots, do
	TaskValueSet 3,2,0					// TaskValueSet TASKnr,VARnr,Value, Reset the Flow counter to 0
	Monitor GPIO,14
	Let,1,0
	Let,2,0
	TaskValueSet 5,1,999999
	TaskValueSet 5,2,999999
	TaskValueSet 5,3,999999
	TaskValueSet 5,4,999999
EndOn

On Clock#Time=All,00:00 do					
	TaskValueSet 3,1,[Watermeter#Total]			
EndOn

On Watermeter#All do						// When Pulse is detected
	if %eventvalue3% <= [DomoticzWatermeter#MinTime] 
	TaskValueSet 3,3,%eventvalue3%	// copy time to MinTime
	endif

		if [Watermeter#Count] = 1 				// if pulse is received
		GPIO,12,0 				 				 				// set second pulsecounter to low
		Let,1,[Pulse#Time]       // set pulsetime from second pulsecounter
		Taskvalueset 5,1,[VAR#1]   //set PulseLow
		Publish domoticz/in,'{"idx":344,"nvalue":0,"svalue":"[Watermeter#Total]"}'
		TaskValueSet 3,2,60000/[Watermeter#Time]	// set Water#Flow
		Publish domoticz/in,'{"idx":338,"nvalue":0,"svalue":"[DomoticzWatermeter#Flow]"}'
		TaskValueSet 3,4,[Watermeter#Total]-[DomoticzWatermeter#CounterTotal]
		endif
		
		If [VAR#1] < [Pulsetime#MinLow]
		TaskValueSet 5,3,[VAR#1]	// set MinLow
		endif
	
EndOn

On GPIO#14=1 do						// When Pulse is rising
		GPIO,12,1 				 				 				// set second pulsecounter to high
		Let,2,[Pulse#Time]       // set pulsetime from second pulsecounter
		Taskvalueset 5,2,[VAR#2]   //set PulseHigh
		If [VAR#2] < [Pulsetime#MinHigh]
		TaskValueSet 5,4,[VAR#2]	// set MinHigh
		endif
Endon
Last edited by GravityRZ on 18 Feb 2021, 12:47, edited 1 time in total.

TD-er
Core team member
Posts: 4272
Joined: 01 Sep 2017, 22:13
Location: the Netherlands
Contact:

Re: Pulse Counter P003 enhancement. How to give back?

#13 Post by TD-er » 18 Feb 2021, 12:46

Just one more optimization tip.
Place the event blocks that happen most often at the top of the rules file.
This may cause the rules parsing to take less time to run.
Longer execution times of anything being executed on the ESP may cause issues on timing critical things like pulse counting.

GravityRZ
Normal user
Posts: 155
Joined: 23 Dec 2019, 21:24

Re: Pulse Counter P003 enhancement. How to give back?

#14 Post by GravityRZ » 18 Feb 2021, 12:49

good tip, did not know that

they are now in the order of happening at bootup (bootup, mqttconnected, mqtt received, pulse counter action)

also i mix gpio triggers with event triggers and i know that is not ideal.

Edit. i noticed i made a new version to compensate for this

Code: Select all

new version with both GPIO pulse
nadeel is dat eerdere waardes verstuurd worden en er een mismatch ontstaat tussen esp en domoticz

On System#Boot do     						// When the ESP boots, do
	TaskValueSet 3,2,0					// TaskValueSet TASKnr,VARnr,Value, Reset the Flow counter to 0
	Monitor GPIO,14
	Let,1,0
	Let,2,0
	TaskValueSet 5,1,999999
	TaskValueSet 5,2,999999
	TaskValueSet 5,3,999999
	TaskValueSet 5,4,999999
EndOn

On Clock#Time=All,00:00 do					
	TaskValueSet 3,1,[Watermeter#Total]			
EndOn

On GPIO#14=0 do						// When Pulse is detected
	if [Watermeter#Time] <= [DomoticzWatermeter#MinTime] 
	TaskValueSet 3,3,[Watermeter#Time	// copy time to MinTime
	endif

	GPIO,12,0 				 				 				// set second pulsecounter to low
	Let,1,[Pulse#Time]       // set pulsetime from second pulsecounter
	Taskvalueset 5,1,[VAR#1]   //set PulseLow
	Let,3,[Watermeter#Total]+1   //added+1 because gpio sends older data
	Publish domoticz/in,'{"idx":344,"nvalue":0,"svalue":"[VAR#3"}'		
	TaskValueSet 3,2,60000/[Watermeter#Time]	// set Water#Flow
	Publish domoticz/in,'{"idx":338,"nvalue":0,"svalue":"[DomoticzWatermeter#Flow]"}'
	TaskValueSet 3,4,[Watermeter#Total]-[DomoticzWatermeter#CounterTotal]+1//added   +1 because gpio sends older data
			
	If [VAR#1] < [Pulsetime#MinLow]
	TaskValueSet 5,3,[VAR#1]	// set MinLow
	endif
EndOn

On GPIO#14=1 do						// When Pulse is rising
	GPIO,12,1 				 				 				// set second pulsecounter to high
	Let,2,[Pulse#Time]       // set pulsetime from second pulsecounter
	Taskvalueset 5,2,[VAR#2]   //set PulseHigh
	If [VAR#2] < [Pulsetime#MinHigh]
	TaskValueSet 5,4,[VAR#2]	// set MinHigh
	endif
Endon

TD-er
Core team member
Posts: 4272
Joined: 01 Sep 2017, 22:13
Location: the Netherlands
Contact:

Re: Pulse Counter P003 enhancement. How to give back?

#15 Post by TD-er » 18 Feb 2021, 13:08

You still have the System#boot at the top and I truly hope this isn't happening frequently ;)

Kim_Panda
Normal user
Posts: 11
Joined: 25 Jan 2021, 17:03

Re: Pulse Counter P003 enhancement. How to give back?

#16 Post by Kim_Panda » 22 Feb 2021, 21:38

Hi, I'm back again and thanks to GravityRZ for sharing your rule solution. It is an interesting and tricky way.
I have not tried it yet, because I am still following my original approach to understand and enhance the plugin 003 code.
If I fail, I'll come back to your approach.

Over the last two weeks I made an enhancement design for the P003 plugin and are now ready with coding and started testing.
The basic concept which I am following is:
- Keep the legacy functionality of P003
- Add further "PULSE" Mode Types which, when an interrupt on the GPIO occurs, waits the debounce time and than reads, with another shorter delay, three times the the GPIO signal. Only when a signal was all three times identical (HIGH/LOW), the signal is regarded as stable and it is counted. Around that functionality is the handling of unexpected situations and retries. In addition I am collecting statistical information for tuning the debounce time parameter and to detect problem situations. The solution shall, if it works, handle debounce problems and disturbing short spike pulses within a pulse.

And here now is a problem, where I hope to get help, probably from @TD-er.
I want to use your Schedule functionality, but it seems not to work (it does not trigger).
I found your explanation in viewtopic.php?f=6&t=5900&p=32089&hilit= ... mer#p32089.
In my case, the Interrupt hander (ISR) is calling
Scheduler.setPluginTimer(debounceTime, PLUGIN_ID_003, 1);
and I expected, that after debounceTime (e.g. 300ms) the function PLUGIN_TIMER_IN is called.
But that does not happen. I also tried PLUGIN_ONLY_TIMER_IN, but it is also not called.
Question: Why? :?:
In PLUGIN_DEVICE_ADD there is Device[deviceCount].TimerOption = true;
Is "TimerOptional" needed here also or anything else?
I had branched "mega" on 7.2.2021.

... and yes, @TD-er, with your hints (thank you!), I managed to fork and branch "head" of "mega" from GIT and I am developing on it now with VS-code(PlatformIO).
Although I am meanwhile in the software business over 40 years, I am unfortunately not familiar with GIT and the handling and procedures around. So please excuse my possibly stupid behaviour. I will be happy to get hints an advice, how to use it and how to interact with the community.

Kim_Panda
Normal user
Posts: 11
Joined: 25 Jan 2021, 17:03

Re: Pulse Counter P003 enhancement. How to give back?

#17 Post by Kim_Panda » 22 Feb 2021, 21:54

Stop, Stop :!:
I got the scheduling working :) after a further compile. I do not know why :roll: . PLUGIN_ONLY_TIMER_IN is called now.
I will come back, when I got it tested/working.
I may then need help to merge my code with parallel updates push my results to you.
Thanks for now!

TD-er
Core team member
Posts: 4272
Joined: 01 Sep 2017, 22:13
Location: the Netherlands
Contact:

Re: Pulse Counter P003 enhancement. How to give back?

#18 Post by TD-er » 22 Feb 2021, 22:49

Just make sure you're working on a branch, or else you may get unpleasantly surprised if you try to rebase the 'mega' branch from letscontrolit over your own 'mega' branch and see the commits "disappear"

What helped me a lot is to work with a graphical Git tool like GitKraken.
I was working with Git a few years on my job and still had to ask this colleague of mine when I got stuck in Git (command line) again and again.
But since I used GitKraken, I finally got the principe of Git.

Just one advice if you're still struggling with Git, is to push commits to your own fork before trying to get in sync again.
If you even created a pull request (which will not get merged immediately ;) ) you can on the GitHub page press a single button to merge the origin 'mega' branch into your PR branch.

Kim_Panda
Normal user
Posts: 11
Joined: 25 Jan 2021, 17:03

Re: Pulse Counter P003 enhancement. How to give back?

#19 Post by Kim_Panda » 23 Feb 2021, 00:21

Thanks a lot TD-er. I will try, when I got it running.

While testing, I just came across a strange thing.
I put a logging inside the main function "boolean Plugin_003(..." for logging the "function" and the "event->TaskIndex".
I found, that all functions are called with event->TaskIndex = 0 except two, which are called with event->TaskIndex = 12 :shock:
The first exception is the very first function call of function 6 (PLUGIN_DEVICE_ADD) and the second exception (which brings me into trouble) is function 33 (PLUGIN_ONLY_TIMER_IN).
Why are these two are called with 12 while all other are called with 0 :?:
Is that a bug or by intention?
I have now the problem, that the ISR writes the variables for index 0 and e.g. PLUGIN_READ processes correctly index 0 while my new PLUGIN_ONLY_TIMER_IN processes index 12, which obviously does not fit.

Can you please advice. Thanks.

TD-er
Core team member
Posts: 4272
Joined: 01 Sep 2017, 22:13
Location: the Netherlands
Contact:

Re: Pulse Counter P003 enhancement. How to give back?

#20 Post by TD-er » 23 Feb 2021, 10:10

Sounds like you need to check for "validTaskIndex"
If a taskindex is not set (intentional not set) it is INVALID_TASK_INDEX, which is TASK_MAX.
Therefore I do have the check validTaskIndex.

Kim_Panda
Normal user
Posts: 11
Joined: 25 Jan 2021, 17:03

Re: Pulse Counter P003 enhancement. How to give back?

#21 Post by Kim_Panda » 23 Feb 2021, 16:23

Ahh, I think I got it now, after I looked, how it is implemented in Scheduler.cpp.
I was mislead by the 5 years old :shock: explanation in viewtopic.php?f=6&t=5900&p=32089&hilit= ... mer#p32089. this is not valid any more.
What I found from Scheduler.cpp is the following. Please correct me, if I am wrong:

There are two different timer possibilities:
  • a) You want to trigger a device's task : That is started by setPluginTaskTimer which comes back as PLUGIN_TIMER_IN
    The device's task is identified in the second parameter of set function by the taskIndex.
  • b) You want to trigger a whole device: That is started by setPluginTimer which comes back as PLUGIN_ONLY_TIMER_IN
    The device is identified in the second parameter of set function by the pluginID.
The functionality of both is basically identical with the difference, that a) returns within the event structure (myTaskIdx = event->TaskIndex;) the given taskIndex while b) returns 12=INVALID_TASK_INDEX
The Parameters Par1 to Par5 from the set function are also returned in event structure (myParam = event->Par1;)

So I first wrongly tried b) but I needed a).
Based on that, I got my code basically functioning :) and can continue with my testing.
I'll come back later ...

TD-er
Core team member
Posts: 4272
Joined: 01 Sep 2017, 22:13
Location: the Netherlands
Contact:

Re: Pulse Counter P003 enhancement. How to give back?

#22 Post by TD-er » 23 Feb 2021, 20:38

Not sure what you want to achieve.
But please also have a look at code like this which is used in a number of plugins (e.g. BME280)

Code: Select all

Scheduler.schedule_task_device_timer(event->TaskIndex, millis() + 10);
This is used to let a task reschedule itself so it will call a PLUGIN_READ and thus deliver new values which will then also be sent to any connected controllers.

Kim_Panda
Normal user
Posts: 11
Joined: 25 Jan 2021, 17:03

Re: Pulse Counter P003 enhancement. How to give back?

#23 Post by Kim_Panda » 24 Feb 2021, 00:41

Ahh, I was not aware of this functionality.
In my case (I'll explain below), I think, the combination of setPluginTaskTimer and PLUGIN_TIMER_IN is more appropriate, as I do not want to automatically trigger connected controllers.
And I am very happy :) that I got it immediately perfectly running, after I once understood the functionality as described in my previous post. I have intensively manually tested today and prepared some documentation. Next is my production test at my gas meter over several days.

Here a summary of my implementation, based on P003 generic Pulse Counter:
  • For legacy modes keep the same processing as before inside the IRQ Handler (i.e. They count pulses if the distance between a new interrupt and the previous counting is longer than debounce time).
  • A set of three new "PULSE" Mode Types is now introduced which work as follows:
    For these modes it is checked if, after debounce time, the same GPIO status can be read three times and only then it counts the pulse.
    In detail:
    When an interrupt on the GPIO occurs it starts in the ISR only a timer (setPluginTaskTimer) for the debounce time and ignores for the time being all further interrupts. When time is over, the Scheduler calls the processing step1 in PLUGIN_TIMER_IN.
    This step1 reads the current GPIO status (high/low) and starts again the setPluginTaskTimer, this time with DebounceTime/2 and for processing step2.
    This step2 again reads the current GPIO status (high/low) and checks, if it is the same as before.
    If no, it does a retry (re-schedules itself after DebounceTime/2)
    If OK, it again starts the setPluginTaskTimer with DebounceTime/2, now for processing step3.
    This step3 again reads the current GPIO status (high/low) and checks if it is the same as before.
    If no, it does a retry (schedules setp2 after DebounceTime/2)
    Now when three times the same GPIO status has been read, it checks if this is the opposite status to the previous pulse detection process (pulse must toggle).
    If no, it ignores it, as the previous pulse is simply confirmed (e.g. after a spike).
    If yes, a new pulse level was finally detected. It now increments counters and resets a processingFlag which tells the ISR that it can trigger the next processing loop if a new interrupt happens.
  • Around that functionality is the handling of unexpected situations and retries. In addition I am collecting statistical information for tuning the debounce time parameter and to detect problem situations.
The solution is intended to handle debounce problems and disturbing short spikes within a pulse.

I hope that explanation makes my project clear. It is not easy to describe correctly and brief at the same time.

I am happy, that I could achieve that inside ESPEasy and I am happy that I found this super framework. It is clearly structured, change friendly and very stable. Many thanks a lot to the Developers!

I am happy to provide you with my full code, which has a lot of describing comments.
But I could not yet find my way, how to do that correctly from my GIT-branch. This will be, after I set up my production test, my next action. For the time being, I will attach the zipped source file, if want to have a look at it.

I wonder, if it makes sense to prepare some user documentation for the new functionality. I am thinking e.g. of the above overview and some more docu about the statistical information that is sent in the logfile and its interpretation for tuning the debounce time and problem detection. Could that be interesting and is there a place for that?

I will come back with my results from my production test and hopefully with a proper pull request. :roll:
Attachments
_P003_Pulse_23.02.2021.zip
_P003_Pulse.ino - enhanced version with PULSE mode types
(6.46 KiB) Downloaded 2 times

TD-er
Core team member
Posts: 4272
Joined: 01 Sep 2017, 22:13
Location: the Netherlands
Contact:

Re: Pulse Counter P003 enhancement. How to give back?

#24 Post by TD-er » 24 Feb 2021, 10:27

You should be aware of a number of things when approaching pulse counting like this.
ESP8266 only can act on pulse rise or fall via an interrupt, so you must do all statistics yourself.
But this also has some "dangers" as the callback code from an interrupt has very strict rules and some things you really should know before using it or else you may run into crashes or slow down the unit a lot (!!)

Callback code is also referred to as "ISR" code.
This means you should only access variables that are marked "volatile" and use functions that are either "inline" or marked with "ICACHE_RAM_ATTR"
If you don't meet these requirements, you will see crashes with very obscure errors.

Another thing you should know is that you should spend as little time as possible in ISR calls and often interrupts are disabled when entering and enabled when leaving (!!!), which is something you should do yourself.
N.B. disabling/enabling interrupts is not needed if you're doing a single call, like incrementing a counter.

One thing that's really slowing down ISR execution is when you're accessing volatile objects which have a size which is not a multiple of 4 bytes.
For example, accessing a float, uint32_t and even an array of those or a double (8 byte size) is just fine.
But using a bool, int8_t or int16_t is taking a lot longer to execute.
So even if it may look like you're wasting resources, store a boolean value in a 32 bit sized object like uint32_t.

If you need to do statistics on pulses to make it work, please please please consider adding a filter to the input pin using a simple resistor and capacitor.
For example 100 nF + 10k resistor already filter out pulses smaller than roughly 1 msec.

What you could try is mark different callback functions for rising and falling flanks and store the getMicros64() value in both.
The nice part of getMicros64() is that it does not overflow and has microsecond resolution, so you can simply subtract their values when handling them in the normal (not ISR) code.
Then you can evaluate values also by seeing what came first, so you know you missed one if it the latter one came first.
Or you store 2 timestamps on a flank, by simply checking if the first one is 0, store it in the next one.
But I think that's as much as you can get away with in ISR handling.

It has been tested with fan speed measurement with fans running over 10'000 rpm, so you can get away with quite a lot, but remember pulse counter plugin does actually interrupt other code, so frequent interrupts do affect operation of other tasks.

Kim_Panda
Normal user
Posts: 11
Joined: 25 Jan 2021, 17:03

Re: Pulse Counter P003 enhancement. How to give back?

#25 Post by Kim_Panda » 28 Feb 2021, 03:19

Thanks a lot for this good hints.

But first the good message: The version I sent with the previous post is counting since 3 days my gas meter pulses 100% correctly. The optional statistic logging quickly helped me in tuning the debounce time and I see now exactly in which filter step, how many false pulses are filtered. 8-)
In my 3 day test of the new sw filter I see, that, only 4% of the interrupts were valid counted pulses (bouncing) and 1% false spikes occurred within steady signal. So there is a lot of noise, but successfully filtered out by the software. And my counted pulses are actually identical to the calibrated counter in the meter.

That does not mean, that it can't be made better and I will now enhance it with the hints you gave:
Here are the points, I got from your message:
  • This means you should only access variables that are marked "volatile" and use functions that are either "inline" or marked with "ICACHE_RAM_ATTR"
    Good point! I used "Scheduler.setPluginTaskTimer" inside the ISR without further thinking. It is not ICACHE_RAM_ATTR and thus not suited for ISR. I'll take that out.
  • ... in ISR calls and often interrupts are disabled when entering and enabled when leaving (!!!), which is something you should do yourself.
    My understanding (from work with other processors and mainframes) is, that the processor automatically disables interrupts, when the ISR is called and enables them automatically, when the ISR terminates. Often only interrupts from own and lower level are disabled. Is that not the case for ESP8266? :?:
    Provided, that my understanding is right, then the use of noInterrupts() inside an ISR is needless and use of interrupts() is dangerous, because it possibly enables the automatically blocked interrupts too early. My understanding is, that the use of noInterrupts() ... interrupts() is normally in the non-ISR functions which do not want to be interrupted by ISR while doing critical things (time critical things or access to multi-word variables, which might be unexpectedly changed by an ISR).
    Does my understanding not apply to ESP8266?
  • One thing that's really slowing down ISR execution is when you're accessing volatile objects which have a size which is not a multiple of 4 bytes.
    For example, accessing a float, uint32_t and even an array of those or a double (8 byte size) is just fine.
    But using a bool, int8_t or int16_t is taking a lot longer to execute.
    So even if it may look like you're wasting resources, store a boolean value in a 32 bit sized object like uint32_t.
    Good point!
  • If you need to do statistics on pulses to make it work, please please please consider adding a filter to the input pin using a simple resistor and capacitor.
    For example 100 nF + 10k resistor already filter out pulses smaller than roughly 1 msec.
    I will definitely do that later. But my original intension was to let the software handle that, ... if possible.
    I already found from my tests, that a lot of spikes (~1%) that my software filtered out, came from simply switching the room light on and off.
  • What you could try is ... the getMicros64() ... . The nice part of getMicros64() is that it does not overflow and has microsecond resolution, so you can simply subtract their values when handling them in the normal (not ISR) code.
    I did already a quick try, but it is a little tricky to use. I was looking for functioning examples. Is it right, that it is only used in ESPEasyWifi_ProcessEvent.cpp? :?: But I will give it a try.
  • Or you store 2 timestamps on a flank ...
    It has been tested with fan speed measurement with fans running over 10'000 rpm, so you can get away with quite a lot
    Here might be the fundamental difference of my approach. My aim is to count 100% exactly low frequent pulses, like counter pulses from gas or water meters (< 60 rpm and often long times with no pulse at all) and the counted pulses shall 100% fit with the calibrated counter inside the meter. Therefore my approach is to just use the flank as starting trigger, but then, after debounce time, I do pin-reads to check and validate the signal. But definitely, that can only be done because the low frequency gives the time for that "triple check". Only the trigger comes from the ISR, while the multi step checks are done in "normal" functions triggered by the scheduler.
    My approach may not be suited for high frequency e.g. fan speed measurement. There is not the time for triple check and also not the need, because the pulses there come with more or less the same frequency over a longer time and a single false signal is probably not of any significance.
Do you think, my approach is of use for the community and worthy and suited for ESPEasy? :?:
If yes, then my next steps would be:
  • in ISR, only set probably two flags and a timestamp (getMicros64())
  • only use 32bit or multiple data types, even for flags
  • see if dis-/enable interrupt is usefull/needed (cf. my question above)
  • will check the ISR flag e.g. in a "PLUGIN_FIFTY_PER_SECOND" section and do there the "Scheduler.setPluginTaskTimer". Here fore I need one flag.
  • keep doing the filter steps in the "PLUGIN_TIMER_IN" section. Here I need the second flag for not being called again by ISR, while the steps are being processed

TD-er
Core team member
Posts: 4272
Joined: 01 Sep 2017, 22:13
Location: the Netherlands
Contact:

Re: Pulse Counter P003 enhancement. How to give back?

#26 Post by TD-er » 28 Feb 2021, 11:26

The interrupts level used in the ESP for pin activity is also shared for other parts.
So there is a need for disabling it in handling ISR calls that may be slightly more elaborate than just storing a value. (whether that's a good approach is another discussion)

Using 2 timer values can also help to filter out spikes, so even if you're aiming for low frequency counts, you can still use it as a spike detector.

Kim_Panda
Normal user
Posts: 11
Joined: 25 Jan 2021, 17:03

Re: Pulse Counter P003 enhancement. How to give back?

#27 Post by Kim_Panda » 28 Feb 2021, 13:13

So there is a need for disabling it in handling ISR calls
That is very important to know. It sounds odd, but I assume there are good reasons for doing so.
Using 2 timer values can also help to filter out spikes
I see the point and it sounds good, but somehow contradicts to my approach.
With my solution approach, you can select in the Mode Type, what approach you want to use. The new one or the legacy, spike based one.

I would appreciate, if you could give a quick answer for the other two questions ( :?: ) from my previous post. This helps me with my next steps.

TD-er
Core team member
Posts: 4272
Joined: 01 Sep 2017, 22:13
Location: the Netherlands
Contact:

Re: Pulse Counter P003 enhancement. How to give back?

#28 Post by TD-er » 28 Feb 2021, 20:20

will check the ISR flag e.g. in a "PLUGIN_FIFTY_PER_SECOND" section and do there the "Scheduler.setPluginTaskTimer". Here fore I need one flag.
Not sure if you need a flag. You can also clear the variable if you read them.
Just add some very simple 'copyAndClear' like function which does try to do it in almost a single call.
By making a function for it you can be sure the compiler will not try to optimize things into out-of-order execution which makes it possible for an interrupt to intervene.

Something like this.

Code: Select all

uint32_t getAndClear(uint32_t& variable) {
  const uint32_t res = variable;
  variable = 0;
  return res;
}
keep doing the filter steps in the "PLUGIN_TIMER_IN" section. Here I need the second flag for not being called again by ISR, while the steps are being processed
Not sure why you would do stuff in both the PLUGIN_FIFTY_PER_SECOND and PLUGIN_TIMER_IN.
My first response would be to handle stuff in 1 single call and maybe if you still need the other one, to set a flag somewhere.

GravityRZ
Normal user
Posts: 155
Joined: 23 Dec 2019, 21:24

Re: Pulse Counter P003 enhancement. How to give back?

#29 Post by GravityRZ » 01 Mar 2021, 16:24

i will try out the new P003 implementation to see if it also solves my problem(extra pulses)
as soon as i get a mismatch in my current setup i will flash the new fw

quick question
at the moment i have set debounce time to 2400ms (maxflow is 24.9 liters so 60000/25=2400ms)

when i use the new pulse low setting can i use the same debounce time or do i need to set it to 1/2 of that(1200ms)

TD-er
Core team member
Posts: 4272
Joined: 01 Sep 2017, 22:13
Location: the Netherlands
Contact:

Re: Pulse Counter P003 enhancement. How to give back?

#30 Post by TD-er » 01 Mar 2021, 22:20

2400 msec does sound a bit too 'tight'.
What does determine this max flow?
Is it an absolute maximum rotational speed of the flow sensor?
In my experience, an analog sensor (a mechanical sensor is an analog sensor) may perhaps on average have a max flow it will measure, but you can be almost certain the sensor itself may sometimes show higher values which are valid measurements.

As a simple example; What if the flow is suddenly interrupted by closing a valve?
When a washing machine closes the water intake valve you always hear a 'klunk' sound in the water pipes.
This means there is a shock wave in the water pipes, which will also be measured by a flow sensor.
If this triggers a sensor to flip again past a detector a few times, what should you do with it?
Or the other way around, if you water your garden via a garden hose and leave the water running but disconnect the hose. Then the flow will increase quite a bit for a short while to compensate for the difference in pressure in the pipes and maybe some flexible hoses of your watering system.

Or what if you have water running at almost the maximum flow, but your sensor may pick up some very short noise spike?
Then the valid 2500 msec pulse will be rejected since it is now considered one of 2000 msec and one of 499 msec, which both are less than the required 2400 msec minimal pulse length.

So I would perhaps focus more on the worst case of false pulses to filter out (e.g. 100 msec in your situation) and not just try to find the max. expected flow.
The thing with 'expectations' is that in practice you will always see 'unexpected' values :)

Post Reply

Who is online

Users browsing this forum: Bing [Bot] and 22 guests