oem_gateway icon indicating copy to clipboard operation
oem_gateway copied to clipboard

Addition of listener and buffer for new emoncms node interface

Open TrystanLea opened this issue 10 years ago • 12 comments

Hello Jerome

This is a new listener and buffer to work with the new emoncms node interface that needs to oem_gateway to just forward the RFM12 Demo output on without conversion to integers to this API:

http://localhost/master/node/set.json?nodeid=10&data=20,20,20,20

where each comma separated value is a byte value.

There's a lot of repetition with the standard listener and buffer (the new listener and buffer is just copy and paste with minor changes) not sure if the differences could be abstracted, configured out somehow. What do you think? Is it worth it?

TrystanLea avatar Mar 22 '14 13:03 TrystanLea

To give more context to this pull request, Im working on a new home energy monitor build guide that explains the setup of a system using the new node interface and so on, its up here:

http://openenergymonitor.org/emon/node/4283

TrystanLea avatar Mar 22 '14 13:03 TrystanLea

Hi Trystan.

I didn't know about the Node interface. Is there a bit of doc you'd like to point me to ? Your link doesn't seem to deal with the insides of it. Perhaps should I just peek into the code. It's about time I update my emoncms install...

Nice that you put your hands on python and the gateway.

I'm reading your proposal. Here are my comments.

You're right about the fact that there is much code duplication. I designed the gateway trying to avoid this as much as possible. We'll deal with this before integrating.

OemGatewayRFM2PiListenerNode

It is the same as OemGatewayRFM2PiListener, except the values are sent entirely, without the conversion in two integers, just like in the SerialListener.

Since you made OemGatewayRFM2PiListenerNode inherit from OemGatewayRFM2PiListener, you didn't have to duplicate all methods (in OOP, the functions of a class instance are called methods, don't ask me why). You just needed to duplicate the ones you changed.

In the init() method, super(my class, self) refers to the class from which you inherit. (In python3, this gets simplified as super(). Basically, what you do here is call the init method from the class you inherit from, then add your own specific init instructions. But you copied the init() instructions from the parent OemGatewayRFM2PiListener, so you're doing them twice. Since it is just setting values to self._settings, it is not a problem, it is just useless. Remove the self._settings= ... line and then you realize your init method just calls the parent's init, which means it is useless. Just don't create an init() method. By default the parent's init is called.

In fact, the only method you actually change is _process_frame, so it is the only one you need to duplicate. Remove all the other methods. They will be inherited from the parent.

In your _process_frame function, the docstring (initial comment) is not correct. It reads f (string): 'NodeID val1_lsb val1_msb val2_lsb val2_msb ...' and talks about recombining.

Since you accept any numbers without recombination, I assume you may also expect decimal numbers, not only integers. If this is the case, this line is not correct :

# Only integers are expected
received = [int(val) for val in received]

You'd need float() instead of int(), like in OemGatewayListener.

In fact, looking at the readme Listener tree,

OemGatewayListener
  |
  |-- OemGatewaySerialListener
  |     |
  |     |-- OemGatewayRFM2PiListener
  |           |
  |           |-- OemGatewayRFM2PiListenerRepeater
  |
  |-- OemGatewaySocketListener

it looks like your NodeListener sort of stands between OemGatewaySerialListener and OemGatewayRFM2PiListener. (BTW we should update the readme when integrating the new listener.)

OemGatewayListener is the simplest with default functions. In fact, it is "abstract", useless by itself, rather a placeholder for real implementations, with a few default methods. SerialListener adds the serial port management. OemGatewayRFM2PiListener adds RFM2Pi specificities : RFM module management (freq, group, etc), broadcast time, and recombination of numbers according to the protocol.

Since the Node Listener needs some of these features but not all, the inheritance could be done the other way around.

OemGatewayListener
  |
  |-- OemGatewaySerialListener
  |     |
  |     |-- OemGatewayRFM2PiListenerNode
  |           |
  |           |-- OemGatewayRFM2PiListener
  |                 |
  |                 |-- OemGatewayRFM2PiListenerRepeater
  |
  |-- OemGatewaySocketListener

It would provide the methods the OemGatewayRFM2PiListener currently provides, except for _process_frame(). Then OemGatewayRFM2PiListenerNode would inherit from it and add _process_frame() for the recombination.

We'd need to check that _process_frame() from SerialListener (which is in fact the default one from OemGatewayListener) is fit for our needs.

The differences between your Node _process_frame() and the one from OemGatewayListener are :

  • The comments, but there, OemGatewayListener is correct and Node is wrong
  • The lines to skip information messages from the RFM. This is not my favourite part. I think you see the meaning of it. It is far from perfect, incomplete, and I don't have the specs to make it complete (or I didn't find them), which is why there are still messages passing through and generating warnings. Yet, it should be safe to put in the OemGatewayListener's _process_frame(). Not nice, but safe. Nicer (best, in fact) would be to create a new dedicated filter method called from _process_frame() and implemented only in OemGatewayRFM2Pi* listeners.
  • Number of elements test. You removed the oddness test since there is no recombination, but you can keep the "at elast two" test from OemGatewayListener.
  • Integers/float. I think float is fine (even if you didn't expect floats, it wouldn't hurt).
  • Recombination. You neutralized this piece of code by making it a useless operation : values[] is just a copy of received[]. You might as well return received[] right away, which is what is done in OemGatewayListener.

All in all, we can almost use the _process_frame() method from OemGatewayListener. So I think it makes sense to reverse the inheritance link as I explained. The only think to pay attention to is this information messages skipping. I like the "nice" method.

Is that clear to you ?

I assumed you preferred an explanation to a silent rework. Besides, it is nice you get familiar to the code.

Bell rung. Buffer review later.

Please ask for clarifications if I was unclear.

Bye.

lafrech avatar Mar 23 '14 17:03 lafrech

How do you manage to avoid recombination if the data goes through the radio link ? Do you limit the values to 255 ?

Or if it does not use the radio, then why call it RFM2Pi ? I mean if it is just serial, why not use the Serial listener ?

From what I read here, it does use the radio link: http://openenergymonitor.org/emon/node/4283

lafrech avatar Mar 23 '14 18:03 lafrech

Is there any explanation somewhere about what the Node interface is ? I don't understand how it works through the radio without recombining.

Also, does it contain a way of sending named inputs ? This is what the screenshot suggests.

lafrech avatar Mar 25 '14 21:03 lafrech

Hello Jerome,

Here's the brief bit I have written up about the node interface so far: http://openenergymonitor.org/emon/node/3868

As you know the input interface limited the datatypes that could be sent from sensor nodes up to emoncms to integers.

The idea behind the node interface is that there is no limitation of the datatypes that can be sent you just forward that string of binary values 0-255 as generated by RFM12Demo straight up to emoncms (with a conversion from space seperated to csv).

Emoncms then has the equivalent of the struct defenition in the node interface, so that you can choose to decode the first 2 bytes as an integer and then the next 4 as a long and so on.

Its both more flexible than the input interface and .. it should be easier for new users to setup as you just select from the dropdown menu from a set of standard nodes such as EmonTx v3 (continuous sampling) and that automatically populates the variables with variable names, units and so on.

You can also define a custom decoder, the preset decoders are there just to make things easier

TrystanLea avatar Mar 25 '14 22:03 TrystanLea

the name and units are not sent by the nodes in anyway, its all done via the js interface.

TrystanLea avatar Mar 25 '14 22:03 TrystanLea

Removing the methods, and inheriting as you suggest sounds like a nice way to do it, thanks for the explanation. Shall I try and do that?

TrystanLea avatar Mar 25 '14 22:03 TrystanLea

OK, thanks. It's clear now. This whole thing rings me a bell. I've got a lot of catchup to do on the forums.

Then my assumptions were correct. As I said, the inheritance should be inverted and in the end it is not so many new lines added to the listener.py file. Does what I wrote make sense ? Maybe I can try to find a little bit of time to do the code myself.

I had a very quick look at the Buffer. Kinda mixes up with what I should be doing on the gateway now that you unlocked the API issue. I'd like to take the time to make this cleanly.

Can I just git pull the latest emoncms master and keep MYSQL without installing redis or timestore or anything else ? Or is there a bit of config to do before I'm up to date ? I'm still on this revision: https://github.com/emoncms/emoncms/commit/ac39fdc2232fd879201f63a28c4ad4e4beefbc1a

lafrech avatar Mar 25 '14 22:03 lafrech

Didn't see your last message. Network issues... I can try to do the Listener part one of these days. Unless you're interested and/or in a hurry.

lafrech avatar Mar 25 '14 22:03 lafrech

no not in a hurry, I think you would do a nicer job than me.

You should be ok to upgrade to the latest emoncms master, you will need to create a new settings file and copy your db settings across. You may be safer creating another folder with v8 in it and just testing that you can read all your data first without effecting your monitoring. Then if you dont see any bugs you could then git pull in your main emoncms folder?

TrystanLea avatar Mar 25 '14 22:03 TrystanLea

Trystan, I think I'll integrate Dave's contrib first, as it changes a lot of code, with the buffer -> dispatcher rename.

Then I'll be able to add the new listener.

Before adding the new buffer (now called dispatcher), I'll try to have a look at bulk mode sending and all, now that you changed the emoncms API. Once this is done, I think things will be clearer, and part of the job will be done already.

So, in chronological order, I'll probably do:

  • Dave's contrib (https://github.com/Jerome-github/oem_gateway/pull/21)
  • Bulk sending (https://github.com/Jerome-github/oem_gateway/issues/6)
  • Your pull request (this)

lafrech avatar Apr 06 '14 11:04 lafrech

Great and thankyou. I need to create a bulk sending API for the new node interface too.

TrystanLea avatar Apr 08 '14 16:04 TrystanLea