martenjacobs / py-otgw-mqtt

Python OTGW MQTT bridge
MIT License
9 stars 16 forks source link

Add TCP/IP connection for OTGW #1

Closed jodur closed 6 years ago

jodur commented 6 years ago

I have added the TCP/IP connection for the OTGW. I have tested it with mosquitto MQTT.

jodur commented 6 years ago

Marten, I understand your remarks. Let me explain the changes.

main.py

The subscription to MQTT topics didn't work for me with the original code. After some digging on internet i found out why : https://www.hivemq.com/blog/mqtt-essentials-part-5-mqtt-topics-best-practices

The topics behind root for consists of 2 levels (for example: set/otgw/room_setpoint/temporary ) and the "+" wildcard you were using is only valid for one level. Replacing the '+' with '#' (multi level wildcard) worked for me and i am surprised it worked for you? Also the subscription to the system topics mqtt_client.subscribe("$SYS/#") seems to be useless because these topics are not used in code, so that why i removed it.

timeout setting: I understand why you need the timeout, otherwise the code blocks until 128 bytes is read. This is specific for the serial communication, so it is a parameter that shouldn't be in the READ method of the abstract class OTGWClient, esspecially if a constant (0.5) is passed as parameter. In the Open method the value is already set to 0.1 and with the first READ it is the set to 0.5?? This didn't make any sense in my humble opinion (i am not a expert!). It would be more logic to set this value in the implementation of the READ method within the OTGWSerialClient.

config.json

It would be better to provide 2 config.json files one named config_tcp.json and config_serial.json to provide examples for users how to use the config.json.

martenjacobs commented 6 years ago

Hi Jodur,

After checking them more thoroughly, I understand and agree with your changes to main.py. I don't understand why the wildcard character is working for me either... I think changing the log level of the message received is more in line with the log levels in other places as well. I may add a logging level setting to the config.

As for the config: I think we should show both examples in the README.md and only include one of the options as a separate file, because the code specifically looks for config.json.

The timeout it's not meant to be specific to the serial implementation. It's specified to make sure the _worker method in the OTGWClient object regains control often so it can check if it should still be running. I decided to put the timeout on the read method, because it's a timeout meant for that specific method call. The serial implementation has to work around that because there's only one timeout parameter for the connection. I actually changed the timeout value from .1 to .5 at a certain point, that's why .1 is still in the constructor. Your code will always block until 128 bytes are received, which has a few unwanted effects:

On your comment regarding line endings: in the serial commands section on this page it's stated that the newline character is optional, so including it shouldn't matter.

jodur commented 6 years ago

When you adapt the read method to the following code you achieve the same as in your original code. I didn't have this in the commit i did ;)

The TCP variant doesn't need the timeout because the amount of bytes you pass in the parameter is a maximum of bytes you want to receive. If less are available or zero is will return the available amount of bytes or a null string, so it is a "non-blocking read". The serial read is normally a "blocking read", that why a timeout is needed, so it won't block longer then the timeout.

def read(self):
        r"""
        Read a block of data from the serial device
        """
        self._serial.timeout = 0.5
        return self._serial.read(128)
martenjacobs commented 6 years ago

Hi Jodur,

A socket in python is supposed to be blocking by default, as mentioned in the docs. As your code doesn't change that, you should be using a blocking socket. Both blocking and non-blocking reads are available, but using a blocking read is better in this case, because otherwise we would be waiting actively (at 100% CPU) for data to appear, because the worker calls the read method in a loop without any source of delay other than the read method's timeout.

I know I can set the serial timeout in the read method's body itself, and let it be implementation dependent, but it was a design choice to put it in the arguments for the read method. The reason being that the programmer of the implementation may not know why the timeout argument is there, but it's clear he has to adhere to it.

If you prefer, I could put your implementation into my code as I believe it should be, so you can test it, or you can change your code and submit a new pull request.

jodur commented 6 years ago

Marten,

Please add the code as you think it should be. I will test it.

I looked at the wrong docs version 2.7.14 (https://docs.python.org/2/library/socket.html#), this is a older version of python. The blocking seems to be added in newer versions. Sorry, i already mentioned that i am a python newbie?

I am testing this on a windows machine on phyton 3.5 and the final version has to run on a raspberry pi stretch debian also with pyhton 3.5.

That is the purpose of my commit and comments, so we get a nice piece of software from "one" source and not from several different branches created by individuals.

Concerning the extra "\n" with the send command. The reason i removed it and just use the "\r" is because after sending the command i always got the response "SE". Thats why i decided to use wireshark to sniff the original commands send by the orginal otmonitor programm. It was only sending as "\r" closing character. After aplying this to the code it worked als with youre code.

jodur commented 6 years ago

Marten, I managed to setup my familiar Visual Studio IDE with the additional python tool, so i know can use VS to do the phyton coding. I was using the embedded IDLE editor but that is not comfortable with multiple files. What are your best experiences with a python IDE? After getting a better overview now, one important thing must also be added to the library if we want to run this software as a "never forget" service. exception handling for following situations:

For the above i would normally implement a state machine with states disconnected, connected and communication interrupted In the disconnected state i would do, the open method, after success => State connected if not retry to open the connection after a delay, the read statements in State connected. If a failure is detected=> communication interrupted state, do some error handling (closing sockets for example) in State communication interrupted and then go back to state disconnected. I think this should be implemented in the worker Thread?

martenjacobs commented 6 years ago

Hi Jodur, I'll try to find the time to build this in the coming week. I'm using Atom for my Python development. For .NET languages, I use VS a lot, but something like Atom is much easier to use and much simpler when writing Python in my opinion. Some people I know like using Eclipse instead, because it has more extensive debugging facilities. I agree that the handling of lost connections could be improved a lot. I created a new issue (#2) where we can discuss that.

martenjacobs commented 6 years ago

I'm pulling this into a separate branch, and will then make the changes I talked about in there