astroufsc / chimera

Chimera - Observatory Automation System
http://chimera.sf.net/
GNU General Public License v2.0
36 stars 19 forks source link

Weatherstation #96

Closed rsouza01 closed 9 years ago

rsouza01 commented 9 years ago
wschoenell commented 9 years ago

@rsouza01 nice job! I would just change one thing... I would move the unit conversion functions to a general function on instruments/weatherstation.py to be inherited by all the weather station drivers. Something like:

def _convertUnits(value, unit_in, unit_out):
    if unit_in == unit_out: return value
    (do the conversion here...)
    return value converted to unit_out

sample call:

_convertUnits(10, Units.CELSIUS, Units.KELVIN) = 283
phsilva commented 9 years ago

And maybe try to use astropy units for the unit conversion (https://astropy.readthedocs.org/en/latest/units/), but not sure if might be overkill at this moment, just don't like the smell of constants rolling around the code.

wschoenell commented 9 years ago

:+1: for the astropy.units use. :-)

tribeiro commented 9 years ago

:+1: astropy.units

wschoenell commented 9 years ago

Something that I see that maybe could be a problem on this interface is the lack of date/time information. Imagine that one have a weather station which sends data only every 10 minutes. Would him want to save the information that the relative humidity was actually from 10 minutes ago? How? temperature() and the other methods returning value, datetime_measurement?

phsilva commented 9 years ago

returning (t, value) looks logical to me, a namedtuple if possible, tuples are just confuse, you never know what value[0] means vs value.t or value.temperature.

rsouza01 commented 9 years ago

System of units changed from proprietary enum to astropy.units.

rsouza01 commented 9 years ago

Named tuples implemented as requested.

wschoenell commented 9 years ago

They make the header look like this: screen shot 2015-08-03 at 16 14 47

rsouza01 commented 9 years ago

Fixes and documentation implemented.

wschoenell commented 9 years ago

@rsouza01,

I think that if you do the minor changes that I just commented on the code and add the correct the units to the metadata output (the [XXX] below), this PR is okay to merge.

METMODEL= 'unknown '           / Weather station Model                          
METRH   = '94.6410161514'      / [%] Weather station relative humidity          
METTEMP = '-6.65063509461'     / [degC] Weather station temperature             
METWINDS= '10      '           / [m/s] Weather station wind speed               
WINDDIR = '90.0    '           / [deg] Weather station wind direction           
METDEW  = '10      '           / [degC] Weather station dew point               
METPRES = '1140.0  '           / [hPa] Weather station air pressure             
METRAIN = '0       '           / Weather station rain indicator                 

I think that it will be something like temperature().unit.name inside that bracket.

rsouza01 commented 9 years ago

All minor changes implemented.

rsouza01 commented 9 years ago

Fixes related to the rain indicator implemented.

tribeiro commented 9 years ago

How is this going? Last update was 6 days ago. Can we merge? :grin:

wschoenell commented 9 years ago

For me it is good to go. If no one manifests against, I will merge next monday.

Tested on LNA with the fake driver configuration:

weatherstation:
  - type: FakeWeatherStation
    name: fake

header example:

METMODEL= 'unknown '           / Weather station Model                          
METRH   = '31.7157287525'      / [%] Weather station relative humidity          
METTEMP = '32.6776695297'      / [degC] Weather station temperature             
METWINDS= '10      '           / [m/s] Weather station wind speed               
WINDDIR = '307.279220614'      / [deg] Weather station wind direction           
METDEW  = '10      '           / [degC] Weather station dew point               
METPRES = '1140.0  '           / [hPa] Weather station air pressure             
METRAIN = '0       '           / Weather station rain indicator