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

Aggregate values #33

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
75 changes: 73 additions & 2 deletions bin/user/mqtt.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,21 @@
# To specify multiple cyphers, delimit with commas and enclose
# in quotes.
#ciphers =

To send aggregated values, define them in section

[StdRestful]
[[MQTT]]
...
[[[calculations]]]
aggobs = period.obstype.aggtype

aggobs: the name you give the aggregated value
period: one of 'day', 'yesterday', 'week', 'month', 'year'
obstype: an observation type in the packet
aggtype: an aggregation to be perfomed on the observation type
There can be multiple lines like that.

"""

try:
Expand Down Expand Up @@ -126,8 +141,10 @@
import weewx.restx
import weewx.units
from weeutil.weeutil import to_int, to_bool, accumulateLeaves
import weeutil.weeutil
import weewx.xtypes

VERSION = "0.24"
VERSION = "0.25"

if weewx.__version__ < "3":
raise weewx.UnsupportedFeature("weewx 3 is required, found %s" %
Expand Down Expand Up @@ -280,6 +297,9 @@ def __init__(self, engine, config_dict):
for obs_type in site_dict['inputs']:
_compat(site_dict['inputs'][obs_type], 'units', 'unit')

if 'calculations' in config_dict['StdRESTful']['MQTT']:
site_dict['calculations'] = config_dict['StdRESTful']['MQTT']['calculations']

site_dict['append_units_label'] = to_bool(site_dict.get('append_units_label'))
site_dict['augment_record'] = to_bool(site_dict.get('augment_record'))
site_dict['retain'] = to_bool(site_dict.get('retain'))
Expand Down Expand Up @@ -390,7 +410,8 @@ def __init__(self, queue, server_url,
post_interval=None, stale=None,
log_success=True, log_failure=True,
timeout=60, max_tries=3, retry_wait=5,
max_backlog=sys.maxsize):
max_backlog=sys.maxsize,
calculations={'dayRain':'day.rain.sum'}):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure whether the unconditional mentioning of rain is correct here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is. See poblabs/weewx-belchertown#685 for explanation.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused by this. Is this then a bug in the MQTT addon, which needs to be solved at the root rather than considered in your calculations contribution?? @matthewwall your opinion?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. The general rule of WeeWX is to have an interval, that is open at its left end and closed at its right end. The only exception is dayRain, and it is calculated that way because of the special needs of the the WU upload. Within the WeeWX sources there is a comment on this that is

                # NB: The WU considers the archive with time stamp 00:00
                # (midnight) as (wrongly) belonging to the current day
                # (instead of the previous day). But, it's their site,
                # so we'll do it their way.  That means the SELECT statement
                # is inclusive on both time ends:

For other use cases than the WU upload the daily rain should be calculated in the normal way, that is $day.rain.sum.

The Belchertown skin uses dayRain to display the daily rain, and so it shows a wrong value when it rains just before midnight, if the MQTT extension uses that special calculated dayRain value.

super(MQTTThread, self).__init__(queue,
protocol_name='MQTT',
manager_dict=manager_dict,
Expand Down Expand Up @@ -422,6 +443,7 @@ def __init__(self, queue, server_url,
self.tls_dict[opt] = tls[opt]
logdbg("TLS parameters: %s" % self.tls_dict)
self.inputs = inputs
self.calculations = calculations
self.unit_system = unit_system
self.augment_record = augment_record
self.retain = retain
Expand Down Expand Up @@ -537,3 +559,52 @@ def process_record(self, record, dbm):
if res != mqtt.MQTT_ERR_SUCCESS:
logerr("publish failed for %s: %s" %
(tpc, mqtt.error_string(res)))

PERIODS = {
'day':lambda _time_ts:weeutil.weeutil.archiveDaySpan(_time_ts),
'yesterday':lambda _time_ts:weeutil.weeutil.archiveDaySpan(_time_ts,1,1),
'week':lambda _time_ts:weeutil.weeutil.archiveWeekSpan(_time_ts),
'month':lambda _time_ts:weeutil.weeutil.archiveMonthSpan(_time_ts),
'year':lambda _time_ts:weeutil.weeutil.archiveYearSpan(_time_ts)}

def get_record(self, record, dbmanager):
"""Augment record data with additional data from the archive.
Should return results in the same units as the record and the database.

returns: A dictionary of weather values"""

# run parent class
_datadict = super(MQTTThread,self).get_record(record,dbmanager)

# actual time stamp
_time_ts = _datadict['dateTime']

# go through all calculations
for agg_obs in self.calculations:
try:
tag = self.calculations[agg_obs].split('.')
if len(tag)==3:
Copy link

@ThomDietrich ThomDietrich Sep 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens otherwise? When can the length of tag be other than 3? I like to log a warning for this kind of thing. Especially in this case, where the error would mainly stem from user config error?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it makes sense to create some error message.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved.

# example: day.rain.sum
# '$' at the beginning is possible but not necessary
if tag[0][0]=='$': tag[0] = tag[0][1:]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to have a formatter run over your addition. This is just one example

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you address with that comment? I unfortunately do not have a formatter.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your code editor probably has one but you could also e.g. use https://codebeautify.org/python-formatter-beautifier

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only write small extensions and PRs. And I do so using a simple text editor within a terminal session. I am no trained Python programmer. I am mechanical engineer, only. So have mercy.

# time period (day, yesterday, week, month, year)
if tag[0] not in MQTTThread.PERIODS:
raise ValueError("unknown time period '%s'" % tag[0])
ts = MQTTThread.PERIODS[tag[0]](_time_ts)
# If the observation type is in _datadict, calculate
# the aggregation.
# Note: It is no error, if the observation type is not
# in _datadict, as _datadict can be a LOOP packet
# that does not contain all the observation
# types.
if tag[1] in _datadict:
# get aggregate value
__result = weewx.xtypes.get_aggregate(tag[1],ts,tag[2],dbmanager)
# convert to unit system of _datadict
_datadict[agg_obs] = weewx.units.convertStd(__result,_datadict['usUnits'])[0]
# register name with unit group if necessary
weewx.units.obs_group_dict.setdefault(agg_obs,__result[2])
except (LookupError,ValueError,TypeError,weewx.UnknownType,weewx.UnknownAggregation,weewx.CannotCalculate) as e:
logerr('%s = %s: error %s' % (obs,tag,e))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of these "fuck it, let's catch all" clauses. Not saying it is wrong in your particular case. Would you be able to separate them out?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me it is no use to send WeeWX to crash if there is some little problem with some little extension.

Copy link

@ThomDietrich ThomDietrich Sep 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly, but not the point.
If you are aware that line 123 might raise a LookupError, handle it there. Rather be specific and handle problems where they arise.

Please don't take this in a demotivating way.

Copy link
Author

@roe-dl roe-dl Sep 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stupid question: So you suggest to repeat all the error handling code (in this case one line, only) for all the places where errors can occur?

In this case I want to make sure, that the code of this function never ever crashes WeeWX. If I do not want to use that "catch all" clause, I would have to surround every single line of code by a try: - except: clause. Don't you think that is a lot of code and difficult to read?


return _datadict
3 changes: 3 additions & 0 deletions changelog
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
0.25 19apr2022
* added calculation and output of aggregated values

0.24 16apr2022
* renamed option 'units' to 'unit', although either will be accepted.

Expand Down