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

Improve WS1 driver code #86

Merged
merged 18 commits into from Jan 7, 2016

Conversation

Projects
None yet
3 participants
@Talon1024
Copy link
Contributor

commented Dec 18, 2015

Add code to the WS1 driver that allows the station to be connected via TCP/UDP, and separates the Station class into classes for data parsing, serial connection, and TCP/UDP connection.

This new code may need further testing and modification before it is ready to be added to the main code base.

@Talon1024

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2015

It should be noted that this code is already in use on our weather station, and I am getting no errors so far I am sometimes getting errors with unexpected header bytes, but I have no idea why, especially because I have the code set up to search for the "peet bros." header before trying to parse the rest of the data in that packet.

Talon1024 added some commits Dec 21, 2015

WHY
Why won't it work

ahem... Turned on DEBUG_READ, and added some more info logging
statements.
@matthewwall

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2015

kevin, nice work so far.

apparently the ws1 do not behave exactly like an ultimeter (see the comments in the ultimeter.py code for some details about how the ultimeter differs from the ws1). also, the ws1 sometimes sends CR-LF, other times just CR (or just LF - i do not recall). anyway, as with most hardware, its good to let things run a good while.

what kind of serial-to-tcpip hardware are you using? is it doing udp broadcasts? or is it in tcp server mode? or tcp client mode?

m

@Talon1024

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2015

Thanks for the comment.

Our Serial-to-TCP/IP setup is described in detail here.

My changes to the driver code make it operate in TCP (or UDP) client mode. The serial-to-ethernet adapter is configured to operate in TCP server mode.

Anyways, I don't think I should be getting repeated "Unexpected header bytes" errors, since I set self.rec_start to False in the StationInet class constructor, and whenever the "Unexpected header bytes" error occurs.

Setting self.rec_start to False should, in turn, cause the driver to continuously receive 8 bytes of data from the device, and then check for the record header within the bytes received. Once the record header is found, self.rec_start is set to True again.

@Talon1024

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2016

I believe my code is stable and robust at this point. We've had weewx running for some time with the code in its current state, got a few timeouts here and there, an unexpected buffer length error once, but it hasn't caused weewx to crash or throw error after error. In addition, I fixed a bug in the original driver code, which failed to consider negative Fahrenheit indoor/outdoor temperatures.

@tkeffer

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2016

Thanks, Kevin.

Matthew is going to have to take a look at this one. Unfortunately, he is without internet for a couple weeks, so it may take a little time.

DEFAULT_TCP_ADDR = '192.168.36.25'
DEFAULT_TCP_PORT = 3000
PACKET_SIZE = 50
DEBUG_READ = False

This comment has been minimized.

Copy link
@matthewwall

matthewwall Jan 6, 2016

Contributor

DEBUG_READ should be an integer, not a boolean. the convention has been to use an integer so that verbosity levels can be specified, e.g., 0 for nothing, 1 for some, 2 for more, etc.

'port', '%s:%d' % (DEFAULT_TCP_ADDR, DEFAULT_TCP_PORT))
else:
# exit(3)
pass

This comment has been minimized.

Copy link
@matthewwall

matthewwall Jan 6, 2016

Contributor

consider this instead:

if con_mode == 'tcp' or con_mode == 'udp':
self.port = ...
else:
self.port = stn_dict.get('port', DEFAULT_SER_PORT)

if you do not like that kind of quiet backward compatibility, then fail hard with an exception indicating that 'mode' is a required parameter.

PACKET_SIZE - len(buf), socket.MSG_WAITALL)
except (socket.error, socket.timeout), ex:
raise weewx.WeeWxIOError(ex)
if DEBUG_READ: loginf("buf: %s" % buf)

This comment has been minimized.

Copy link
@matthewwall

matthewwall Jan 6, 2016

Contributor

any DEBUG_READ messages should be logdbg, not loginf. also, prolly safer to keep it on two lines instead of inlining a conditional.

@Talon1024

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2016

So, where would the messages outputted from logdbg appear? The last time I remember trying to use logdbg, the debug messages didn't appear in the system message log. (/var/log/messages)

# WeeWx IO Errors may not always occur because of invalid data
# DEBUG_READ = True

# This causes an exception

This comment has been minimized.

Copy link
@matthewwall

matthewwall Jan 6, 2016

Contributor

not sure what you mean here. is get_readings or validate_string missing an exception? or is the 'variable referenced before assignment' coming from StationInet.get_readings_with_retry?

This comment has been minimized.

Copy link
@Talon1024

Talon1024 Jan 6, 2016

Author Contributor

WeeWx IO errors could occur because of socket errors. (timeout, address issue, or other issues)

Also, apparently the "buf" variable, defined in the try block, can't be accessed within the except block.

This comment has been minimized.

Copy link
@matthewwall

matthewwall Jan 6, 2016

Contributor

declare buf outside the try block, e.g., buf = ''. be sure to wrap the socket errors into WeeWxIO errors otherwise weewx will die. there should be a generic socket.error or some such which you can catch then raise WeeWXIOError(e)

@matthewwall

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2016

nice work here kevin. a few more things:

  • perhaps put 'import serial' into the two methods that actually use the serial interface (open and get_readings_with_retry). that way someone with a socket-connected device does not have to install any serial packages. (the vantage driver does this)
  • i was hoping to figure out some way to abstract the ethernet-to-serial device into a class that we could put into weewx core so that it could be used with any device with a serial interface. maybe at some point in the future...

btw, as tom noted i will have no internet again for awhile, so apologies in advance for delays in response. we're at cucumber beach (belize) getting water at the moment, but soon we'll be back out to the reef and cayes.

m

if con_mode == 'serial':
self.station = StationSerial(self.port, timeout=timeout)
elif con_mode == 'tcp' or con_mode == 'udp':
self.station = StationInet(self.port, con_mode, timeout=timeout)

This comment has been minimized.

Copy link
@matthewwall

matthewwall Jan 6, 2016

Contributor

for backward compatibility, please consider this instead:

if con_mode == 'tcp' or con_mode == 'udp':
self.station = StationInet(...)
else:
self.station = StationSerial(...)

self.max_tries = int(stn_dict.get('max_tries', 5))
self.retry_wait = int(stn_dict.get('retry_wait', 10))
self.last_rain = None
timeout = int(stn_dict.get('timeout', 3))
loginf('driver version is %s' % DRIVER_VERSION)

This comment has been minimized.

Copy link
@matthewwall

matthewwall Jan 6, 2016

Contributor

i usually put the 'driver version is ...' line as the very first thing in init
that way if there are any failures you always know what version someone is using

@matthewwall

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2016

set debug=1 in weewx.conf to see the logdbg messages. if you still do not see them, then you will have to look at your syslog/rsyslog configuration.

@Talon1024

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2016

OK, I made some more changes to my driver code as per @matthewwall's suggestions.

matthewwall added a commit that referenced this pull request Jan 7, 2016

@matthewwall matthewwall merged commit ea69341 into weewx:master Jan 7, 2016

@Talon1024 Talon1024 deleted the Talon1024:ws1_improved branch Jan 7, 2016

@Talon1024 Talon1024 restored the Talon1024:ws1_improved branch Jan 7, 2016

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.