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

Make WS1 TCP/IP driver code retry connection if it fails. #112

Merged
merged 13 commits into from Apr 20, 2016

Conversation

Projects
None yet
3 participants
@Talon1024
Copy link
Contributor

commented Apr 6, 2016

I made some more changes to the weewx WS1 driver so that it re-tries the socket connection if it fails. The number of attempts and the interval between attempts are specified by the max_tries and retry_wait configuration settings respectively.

I have not tested this, however. But I'm hopeful that this code works as I expect it to.

Make some changes requested by someone else
Bump driver version to 0.25
Import socket globally, as it is part of the Python standard library
Steps towards PEP8 compliance: double spacing between top-level functions and classes.
Add timeout, max_retries, and retry_interval fields to StationInet class.
Retry TCP/IP connection if connection fails. If the connection fails more than a certain amount of times, then error out.
@tkeffer

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2016

Thanks for the PR!

A few comments:

First, before putting this in the code base, it really must be tested.

Second, some of the code could be made more Pythonic, and more in the style of the rest of weewx. For example,

 if isinstance(max_retries, int): self.max_retries = max_retries
    else: self.max_retries = 5

should be simply

self.max_retries = max_retries

The reason is that the class WS1Driver has already checked that max_tries can be coerced into an int, and has already supplied the default.

Third, exstr serves no useful purpose. The cause of any exceptions has already been explicitly logged, once for each exception.

Finally, there is a reason why

import socket

appears just before each use: not all WS1s use a socket connection. By making the import global, you're requiring that all versions import it. Minor point, I know, as socket is a commonly used module, most likely used elsewhere in weewx, but it's still good style to avoid requiring an import if you don't have to.

Could you make these changes and, especially, test, test, test!

Thanks

@matthewwall

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2016

kevin,

you might want to rename StationInet to StationSocket. *Socket and *Serial are conventionally used for tcp/ip (socket) and serial-based interfaces, respectively.

you don't have to do bounds/type checking in the Station* init method - do that in the driver's init, since that is the interface to the configuration file. that means you can punt all of the isinstance stuff in Station*

keep the log messages succinct but useful. for example:

logerr("Connect failed for %s:%s: %s" % (self.conn_info[0], self.conn_info[1], ex))

you probably do not want logerr for all of those. the 'retrying...' and 'will retry in x seconds' are more suitable as dbg messages, not err messages.

you want errors to show up clearly, but you don't want to pollute the logs.

Talon1024 added some commits Apr 8, 2016

Fix all errors that occur when trying to initialize the driver.
Most of the errors were references to nonexistent variables. I've finally got to testing this driver, and fixed these errors in the process.
@Talon1024

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2016

OK, I believe I've implemented most of the suggested changes, and I've done a small amount of testing with weewx on a VM. I still need to test the changes some more to ensure the driver works as expected if the driver fails to connect the first time.

@matthewwall, regarding the StationInet name choice, sockets are not only TCP/UDP connections to other hosts, but they can also be connections between different processes on Unix-like systems.

@matthewwall

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2016

kevin, i'm confused. the reason i suggested a class name of StationSocket instead of StationInet is for exactly that reason - the socket interface is for sockets, not just tcp/udp connections.

as for the dbg/err messages, please take a look at other drivers with max_tries such as ws23xx.py. distinction between err and dbg may seem like a little thing, but when you try to debug/support remotely it is a big thing, and when you have users who are doing logwatch (and other log monitoring), the log messages and their types really matter. of course, if you have a better pattern, lets review it. m

@tkeffer

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2016

Still importing socket globally.

Still includes unnecessary type check with unnecessary default. Specifically,

 if isinstance(timeout, int): self.net_socket.settimeout(timeout)
    else: self.net_socket.settimeout(3)

Talon1024 added some commits Apr 10, 2016

Implement most suggestions by Tom Keffer and Matthew Wall
Rename StationInet to StationSocket.
Change the logging level of some error/debug messages.
Don't import socket globally.
Allow user to omit the port number.
If the user decides to omit the port number, the default port (3000) will be used.
@Talon1024

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2016

OK, I made sure to implement the changes you guys asked for, and I added a lot of extra validation in the WS1Driver constructor.

@tkeffer

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2016

I think you're over-engineering this.

There's a general principle in Python called "It's easier to ask for forgiveness than permission" (EAFP). It means that a Python program should assume that everything is going to be OK, but be prepared to catch an exception should it not be. This should be compared to the C philosophy of "look before you leap," (LBYL) where the caller is expected to provide valid parameters and the program might crash if you don't.

Python was carefully engineered around EAFP, and all good Python programs, including weewx, take full advantage of this.

For example, your driver should not make assumptions about the valid form of a port. That's for the operating system to decide. If the port proves to be invalid, the underlying OS interface will raise an exception. If it doesn't complain, you're good. This way, your code will work on multiple OS's, including those you did not have in mind when you wrote the driver.

I should note that for this example, an invalid port, you should probably not catch the exception at all, because this is a configuration issue, not a runtime error. Let the exception bubble up through the stack.

You should also not change invalid parameters, such as the number of retries, into valid values. If an exception is raised, again, let it bubble up through the program. It might, and probably will, cause program termination, but that's better than making assumptions that you have a better idea of what the user actually wanted.

When you're done, you will have a much simpler driver that will be more reliable, easier to port, and easier to maintain.

@Talon1024

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2016

OK.

I was taught to ensure every value is valid before using it. This can be important, not only in languages like C, but also in more loosely typed languages, like JavaScript, where an invalid numeric string can give you NaN instead of throwing an exception like in Python.

It can also be important when the only valid values are values from a pre-defined set, such as the connection mode string for this driver, or when the value has to meet specific criteria that isn't checked at a lower level, such as whether an object in a scene is hidden or not (my Wing Blender plugin does this).

I'm not catching invalid port number exceptions, but if the user doesn't provide a valid port number after the IP address, in the IP:Port form, the default port (3000) is used.

I've removed the extra code to validate values and change invalid values to valid ones, but I've also added an exception if the user's connection mode string is invalid (i.e. not tcp, udp, or serial).

@tkeffer tkeffer merged commit 0400a01 into weewx:master Apr 20, 2016

@tkeffer

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2016

Much better.

Python is different from C, particularly in how it handles errors. It's also different from JavaScript, which relies on a mishmash of parameter validation and untyped exceptions.

I've made some changes to your branch, mostly to bring parameter names in closer concordance to the rest of weewx. I also allowed floating point retry delays. Check it over and make sure I didn't mess anything up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.