Add digital input handling, including pulse counting

[izak]

Implement a pulse counter in Venus. The people on the African continent want it for water pumps. See below.

Either add it to dbus_adc or make something new.

Details:

• One config: liters per pulse. From 1 liter per pulse up to 100 per pulse.

• The pulse is generated by a mechanical rotating system, which can also stop right in the middle of the pulse. So the software should respond to the edge, or perhaps state change. Either the upgoing edge or the downgoing edge.

• I'm not sure if the digital inputs are connected to a pulse counter implemented in hardware, probably not.

• It's also not necessary: the maximum expected frequency is 5 or 10Hz.

• io configs: lets do the effort of making it external, and in machine run time config, now. (Iet the daemontools run script read it from /etc/venus when it starts the process)

• and add the simple input functionality. This is for inputs not connected to flow meters where instead of counting the pulses and calculating volumes, we instead generate an alarm.

[mpvader]

some comments from during the design phase of the pcb: https://github.com/victronenergy/venus/wiki/bbb-gpio#pulse-counting-with-the-digital-inputs

[mpvader]

https://github.com/izak/dbus-pulsecount

[izak]

Last polish done, now it needs a recipe in the build process.

Quick bit of performance testing: Pushing a button as fast as I can on one gpio raises cpu use to 2%. Probably still need to rig it up to an arduino and run 5 pins interleaved at 10hz or similar to get a good number, though this is about as efficient as it comes: It is interrupt driven.

[izak]

Some details on the implementation. It registers the service name com.victronenergy.digitalinput, and paths take the form /x/Count, /x/Aggregate, /x/Alarm. Which paths are available depends on the configured function of the input. Aggregate is the actual value you want to display, it's the product of the count times the multiplier, since each pulse could correspond to a multiple of the underlying unit.

The alarm condition toggles between a 0 and a 2. I saw elsewhere in the gui that 1 is a warning and 2 is an error, and went with the same convention, though this is of course easy enough to change if needed.

For each pin you can configure com.victronenergy.settings/Settings/DigitalInput/x, the available settings are Function (0 = disabled, 1 = counter, 2 = alarm), Multiplier (how many units per counted pulse), Unit (0 = liter, 1 = cubic meter, 2 = KWh), and Inverted (whether to count rising or falling edge, and whether alarm condition is high or low input). There is also a Count, which stores the last known value of the counter when the process shuts down. It also stores the counter periodically to avoid losses.

I wonder if we perhaps need a Reset call to turn it back to zero?

[izak]

The GetText dbus call on Aggregate will return the Aggregate value and the configured unit. For cubic meter I actually used the unicode character corresponding to ㎥ (0x33a5), and from the tiny bit of research I did dbus should use utf8 to pass this across, so if you see strange results when using that unit, we may have to revisit this decision.

[izak]

I did a CPU-usage test. 10Hz interleaved pulses (they are around 50ms apart) on three pins. Ran out of dupont jumpers or I'd rig up all 5, still think it gives you a good idea :-)

https://www.youtube.com/watch?v=comUd5lFuFI

Summary: While developing, I pushed the button I had connected as fast as I could by hand, and got it up to around 2% CPU. Testing with 3 inputs makes it about 4%. My estimate for all 5 inputs running at 10Hz, should be around 5%, maybe 6%.

[jepefe]

I added the pulse counter service in the gui and works fine and stable. I pushed it to WIP_digital_inputs branch. Digital inputs can be enabled in the I/O settings page and then will appear in the "Digital inputs" menu that is available in the device list page.

Make possible to reset the counter would be nice but perhaps is even better would be to make posible to set it a custom value, then users can make the value match the meter readings.

Cubic feet and gallons should be added for contries that uses the imperial system.

Multiplier I found some gas meters that pulses every 0.1m3 and energy meters that do 1000-10000 pulses per kWh so I think the multiplier setting should be a float and set the minimum value accordingly.

DBus service After discussing it with colleagues we think that would be much better to create a separate DBus service per input instead one service with all the inputs in it.

Some of the reasons are:

• Future integration in VRM portal.
• If we want to show all enabled inputs in the device list page(and we do) we need one service per input, otherwise we'll need to add some ugly exceptions.
• Displaying alarms in the gui with a proper description.

Service names can be something like this: "com.victronenergy.accessalarm.dimp_1" for open door alarm "com.victronenergy.gasmeter.dimp_2" for gas meters "com.victronenergy.watermeter.dimp_2" for water meters "com.victronenergy.energymeter.dimp_3" for energy meters ...

This will require an extra setting to set the input type and to define a fixed list of possible input types. We can always add new types with updates if necessary.

[izak]

For simplicity, it only supports watermeter and alarm for now. The function is now part of the dbus service name, and appears/disappears depending on whether it is enabled in the settings.

Service names are in the form com.victronenergy.watermeter.inp_1. The input number is no longer part of the path (since it is now reflected in the service number). It only shows up as /Count, /Aggregate and /Alarm.

Unfortunately it seems I can onle have one VeDbusService object per process (because each one wants to register / and I didn't discover this until it was too late, so I have to get back in there and fix that :-(

[izak]

Okay, it would seem I had a misunderstanding of how dbus names work. When you connect to dbus, you get a unique name, and then you can claim zero or more other names, but all those other names are essentially just aliases for the unique name corresponding to your connection, and whatever objects you publish are available on all registered bus names.

Therefore it isn't possible for a single process to register multiple service names (or bus names) carrying the same information/object paths, unless I use multiple connections to dbus (which is really the only option). For now this is probably what I need to do.

[izak]

To make it work, I had to make it possible to pass a bus connection to the VeDbusService class.

With that small change in place, and using a separate connection for each digital input, it now works as it should: Each input comes up on its own service name (or bus name).

[izak]

@jepefe Can you check if it fits the requirements better now and let me know if something else needs doing? Also, either @mpvader or perhaps @edevries-victron , the linked changes to velib_python needs review, though I think it is benign enough that a cursory glance is perhaps enough.

[jepefe]

@izak I tested it and all works fine, I pushed the gui changes to WIP_digital_inputs_2 branch.

There is a couple of things that I still missing.

Dbus paths:

• ProductName: Product name, for example "Water meter" or "Digital input alarm"
• /Mgmt tree, all the services have this for the device page. Following paths will be enough
  • /Mgmt/Connection: "Digital input 1", "Digital input 2"...
  • /Mgmt/ProcessName: "dbus-pulsecount"
  • /Mgmt/ProcessVersion: version

For the alarm function "/Mgmt/Connection" is specially necessary to be able to show the input that triggered the alarm in case that more than one input is set to alarm function.

Missing units, please add the following ones:

• 0: Cubic metres
• 1: Litre
• 2: Imperial gallon
• 3: U.S. gallon

As it doesn't need any kind of conversion I implemented it on the gui side but is much better to be have it on the service then GetText will return the value with the correct unit.

Regarding to the velib_python changes, I didn't experience any issue, also tested it with other services but @edevries-victron or @mpvader should check it.

[edevries-victron]

@izak Just pushed the velib_python changes.

[izak]

Regarding units: Liter is there, as the abbreviation 'l'. Cubic meter is also there. I used the unicode ㎥ character (0x33a5). If plain text is better I can do that? I did not test if the gui displays it correctly.

Regarding the other two units: I recall briefly discussing it with Matthijs in July, and we decided not to add more units unless there is a demand for them. It is easy enough to do so if we need to, and if the consensus has changed since then, by all means I will add them now.

I'll look into the Mgmt tree next.

[mpvader]

@jepefe are we using gallons elsewhere?

[jepefe]

@mpvader Yes, we use gallons and cubic metres for the tanks sensors.

[mpvader]

Hi both. The end goal (maybe next year) is to have one system wide setting, and let the gui handle unit conversions to the user preference. So, with that in mind, and the fact that @jepefe has already done this but then just for only the pulse continuing, let's keep this all out of the pulsecount python code.

[mpvader]

The GetValue should return a SI unit. And what the gettext returns is then not relevant since the gui won't use it anyway. Let me know if I made a logic error in this reasoning!

[izak]

I added the /Mgmt tree and the Product name/id. For the ID I used 0xFFFF (not set) for now. Should we allocate one, perhaps 0xA163 for Generic Digital Input?

[mpvader]

@izak for the ADC we've assigned two specific product ids:

        <value>0xA161</value>
        <symbol>VE_PROD_ID_TANK_SENSOR_INPUT</symbol>
        <description>Generic tank input</description>
        <name>Generic Tank Input</name>

        <value>0xA162</value>
        <symbol>VE_PROD_ID_TEMPERATURE_SENSOR_INPUT</symbol>
        <description>Generic temperature input</description>
        <name>Generic Temperature Input</name>

So lets define two productids:

product id 0xA163 - Generic pulse meter input
dbus service name: com.victronenergy.pulsemeter.*
Paths:

/Type (0=Water. This path is read-only. Changing is via the localsettings(?))
/Counts
/Volume

product id 0xA164 - Generic digital input
dbus service name: com.victronenergy.digitalinput.*
Paths:

/Type: 0=Door alarm, 1=Bilge alarm, 2=Burglar alarm, 3=Smoke alarm, 4=Fire alarm, 5=CO2 Alarm. This path is read-only. Changing is done via localsettings (?))
/State: 0=ok, 1=unused, 2=alarm (and there is a entry in localsettings for inverting)

or

/Type: 0=Door alarm, 1=Bilge alarm, 2=Burglar alarm, 3=Smoke alarm, 4=Fire alarm, 5=CO2 Alarm. This path is read-only. Changing is done via localsettings (?))
/State: 0=open, 1=closed (and there is a entry in localsettings for inverting)
/Alarm: 0=ok, 1=unused, 2=alarm

or

/Type: 0=Door alarm, 1=Bilge alarm, 2=Burglar alarm, 3=Smoke alarm, 4=Fire alarm, 5=CO2 Alarm. This path is read-only. Changing is done via localsettings (?))
/State: 0=low, 1=high
/State2= 0=low, 1=high, 2=open, 3=closed, 4=inactive, 5=active, 6=off, 7=on, etcetera. With a setting in local settings telling the python service how to translate from /State to /State2
/Alarm: 0=ok, 1=unused, 2=alarm

or

/Type: 0=Door alarm, 1=Bilge alarm, 2=Burglar alarm, 3=Smoke alarm, 4=Fire alarm, 5=CO2 Alarm. This path is read-only. Changing is done via localsettings (?))
/State: GetValue: 0=low, 1=high
           GetText: low, high, open, closed, inactive, active, off, on, etcetera.
           in the gui there will then be a switch over the string. Disadvantage: then for VRM it is no longer an enum.

/Alarm: 0=ok, 1=unused, 2=alarm

What to do with high/low, on/off, open/closed?

[mpvader]

@izak is there already a plan on how to handle the /DeviceInstance value? It needs to be unique per dbus service (first three parts). In other words, there can't be two com.victronenergy.pulsemeter.something services with the same DeviceInstance.

[izak]

I simply use the gpio number for DeviceInstance.

[mpvader]

I simply use the gpio number for DeviceInstance.

ok thats fine

[edevries-victron]

@izak How many different gpio numbers could be used for the pulse counter? I'm asking because in modbus TCP we use a 8bit unit ID to distinguish between devices of the same type. The device instance must be converted to the unit ID, so if there are too many possible gpio numbers, we have a problem.

[izak]

Okay, requested changes made over the weekend, and the README is updated.

@edevries-victron It has about an order less gpios than 256, I don't think I've ever seen a dev board with even 50, so we should be okay :)

[izak]

More specifically, out of the box we only map 5 digital pins on the beaglebone. To keep things more or less consistent, I also only mapped 5 on the Rpi, even though it has more. One pin is also configured as an output (in case someone wants to attach a relay), and I also focused on pins that don't do double duty (as SPI or i2c connections), so even though the Rpi has 26 gpio pins max, only a rare crazy hobbyist would use all of them.

[mpvader]

@izak all looks fine to me.

@jepefe pls review & work on the gui again.

[izak]

I changed the name of the script and of the repo. It's now here. Updated the README to indicate that it does a little more than just count pulses. Also fixed one annoying bug that only shows up the very first time it starts.

[jepefe]

@izak Please make the GetValue of /Type return the numeric value instead the name string, you can use the get text for that.

@mpvader Do we want to show the digital input alarms in the notifications page? Then we need a path per each alarm type and a setting to set the alarm level:

Warning: Adds a warning to the notifications
Alarm: Adds an alarm and shows the notifications page
/AlarmLevel (0 = Disabled, 1 = Warning, 2 = Alarm)

/Alarms/DoorAlarm  (Value = /AlarmLevel when /State == 1)
/Alarms/FireAlarm 
...

[izak]

@jepefe I made the required GetValue change. Thanks for pointing it out.

[mpvader]

@izak pls also add the alarm paths, there are no warnings, just alarms.

[izak]

I had a look at the gui code to get an idea of how this works. It seems that when a dbus service shows up, the 3rd word in the dot-delimited name indicates the kind of service, and then it knows what kind of alarms you can find on such a service.

That differs a bit from what we've done here, as each gpio shows up as its own service, specifically so that it works the same as the dbus-adc inputs. So when configured as an alarm, each service will represent only one alarm, and the exact kind of alarm (eg /Alarms/DoorAlarm) that has to be monitored depends on the type of alarm.

My suggestion is this, that we create another dbus service that aggregates all the inputs into one. In other words, when the process starts it also creates com.victronenergy.digitalalarms and on that service we put all 6 alarm paths even if no gpio is configured to provide such an alarm. Whenever you do configure a gpio pin to serve as an alarm, the /State of that pin will then be multiplexed onto this alarm service (multiplied by 2, so that it is an alarm and not a warning). The advantage to this is if you have multiple Door alarms, they all show up under one path on one service.

On the gui side, all that is then required is watch for digitalalarms and then watch the paths.

Did I read this correctly?

[mpvader]

@izak, I don't understand why you couldn't have an alarm path per service. And in that path there should be something generic. So not /Alarms/DoorAlarm, but /Alarms/Status, or something like that?

Otherwise we can have a quick slack call, you, Jeroen and myself?

[mpvader]

whatever you do, the name of the alarm needs to be translatable, so the string needs to be in the gui anyway

[mpvader]

New proposal:

Paths on the service itself:

/Type:
    0=Door, 1=Bilge pump, 2=Bilge alarm, 3=Burglar alarm
    4=Smoke alarm, 5=Fire alarm, 6=CO2 Alarm, 7=Generator
    This path is read-only. Changing is done via localsettings (?))

/InputState: 0=low, 1=high  (not affected by invert function)

/State:      0=low, 1=high, 2=open, 3=closed, 4=inactive, 5=active, 6=off, 7=on, etcetera.
             To translate /InputState to State, the service uses the
             /Translation and the /Invert/Translation settings.

/Alarm:      0=ok, 2=alarm

Available Settings in localsettings:

/Translation: 0=no/yes, 2=low/high, 4=inactive/active, 5=running/not running
/InvertTranslation: 0=no; 1=yes
/AlarmSetting:  0: don't generate alarms; 1: low==alarm, 2: high==alarm

[mpvader]

Can a user use the custom name idea thats currently being implemented to give their own name to an input?

See #203

[izak]

Current implementation, very close to the above suggestion. First, I decided to do away with the confusing distinction between /Function and /Type. The pulsecounter feature meant for pumps are now type 1. Type 0 means disabled. So:

/Type:
    0=Disabled, 1=Pulse meter, 2=Door, 3=Bilge pump, 4=Bilge alarm,
    5 = Burglar alarm, 6 = Smoke alarm, 7 = Fire alarm, 8 = CO2 alarm, 9 = Generator
    This path is read-only. Changing is done via localsettings)

/InputState: 0=low, 1=high  (not affected by invert function)

/State:
    0=low
    1=high
    2=off
    3=on
    4=no
    5=yes
    6=open
    7=closed
    8=ok
    9=alarm
   10=running
   11=stopped

/Alarm:      0=ok, 2=alarm (remains 0 if not configured as an alarm condition)

The exact value returned by /State depends on the kind of service, for example Door will return 6 or 7. The high value is always odd and the low value is even.

You can invert the /State value by setting the /InvertTranslationconfiguration option.

It also supports /CustomName which simply passes the configuration option to localsettings. If the custom name is blank (the default value), it defaults to the product name.

This is a pretty big overhaul, the code is much neater and more flexible for future changes. I suspect we may want to tweak the names of the supported inputs and their states a bit, but we should have enough to satisfy most people.

[mpvader]

@izak: looks great to me.

@jepefe: can you take it from here again? (for the gui)

[mpvader]

@izak / @jepefe whats the status on this one?

[izak]

It's all good. Closing.