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

Some of the RESTful uploaders need updating to include additional fields #615

Closed
gjr80 opened this issue Oct 22, 2020 · 19 comments
Closed

Comments

@gjr80
Copy link
Contributor

gjr80 commented Oct 22, 2020

A number of the services supported by the WeeWX RESTful uploaders have further developed since the respective WeeWX uploader was first written/last updated. Consequently some uploaders do not support some fields now supported by the services; for example, WOW now accepts soil moisture and soil temperature. In addition, the AWEKAS uploader was based upon the AWEKAS API v2 but the AWEKAS API is now at v4.

Whilst the uploaders continue to work with the mainstream observations some users are now looking to be able to upload the more recently supported observations.

This issue was highlighted in this weewx-user thread.

@gjr80
Copy link
Contributor Author

gjr80 commented Oct 22, 2020

@tkeffer. I have added support for soil moisture and soil temperature to the WOW uploader and have created new branch restful_update. The challenge I have found is that whilst a station may have multiple temperature sensors typically it is fairly clear that a user wishes to upload outTemp and inTemp (where supported). However, it is not uncommon for a station to have multiple soil moisture sensors populating say soilMoist1, soilMoist2,...soilMoistn. So which soil moisture field does the user wish to upload?

I have implemented a default of soilMoist1 but have given the user the ability to override this under [StdRESTful] [[WOW]] with option soil_moist_field. Likewise with soil temperature.

Would appreciate you having a look at the changes in restx.py, whilst I have achieved the aim I feel I have detracted from the graceful flow of the code...

@tkeffer
Copy link
Contributor

tkeffer commented Oct 23, 2020

I wonder if we need something like the sensor maps that the drivers use? It's a common pattern in the uploaders. For example, the AmbientThread has

        'barometer': 'baromin=%.3f',
        'co': 'AqCO=%f',
        'dateTime': 'dateutc=%s',
        'dayRain': 'dailyrainin=%.2f',
        'dewpoint': 'dewptf=%.1f',
        'hourRain': 'rainin=%.2f',
        'leafWet1': "leafwetness=%03.0f",
        ...

We would still need something to specify the formatting (e.g., %.3f), but the types could get mapped. Each mapping would have a default. For example,

  [[Wunderground]]
    [[[uploader_map]]]
        leafWet2 = leafwetness

would override the default that leafWet gets mapped to leafwetness and, instead, it would be leafWet2 that gets mapped.

@gjr80
Copy link
Contributor Author

gjr80 commented Oct 23, 2020

Yes, that was what was going through my mind when I did WOW. I had something like the following in mind:

  [[Wunderground]]
    [[[uploader_map]]]
        [[[[leafwetness]]]]
            source = leafWet2
            format = %03.0f
        [[[[dewptf]]]]
            source = dewpoint3

Map entries are additive, well additive in as much as they don’t define the entire map but rather alter existing entries or add new. A map entry with no source would be used to remove an existing entry. No uploader_map would give you the defaults. Omitting source or format leaves you with the default for that option.
It’s straightforward but a bit bigger than a few minor changes to WOW and AWEKAS. Would aim for inclusion in 4.3.0.

@gjr80
Copy link
Contributor Author

gjr80 commented Oct 27, 2020

As a test case I have been updating the WOW uploader to use the following format:

  [[WOW]]
    [[[uploader_map]]]
        [[[[baromin]]]]
            source = barometer
            format = %.3f
        [[[[dewptf]]]]
            source = dewpoint
            format = %.1f

Have a couple of implementation issues that depend on the behaviour we want:

  1. Is the uploader map definitive or additive. A default uploader map is used if the user does not specify [[[uploader_map]]], should [[[uploader_map]]] replace the default uploader map in its entirety or should [[[uploader_map]]] be used to update the default uploader map. The corollary in some drivers is that we use a sensor_map to replace the default sensor map and sensor_map_extensions to extend/modify the default sensor map. Given the limited number of fields we are talking about with the uploaders and given that most folks will likely just want to change one or two fields, I say keep it simple and make [[[uploader_map]]] additive, ie it just updates the uploader map rather than replace it.

  2. Action if a config option is omitted or a nonsense value. For source I think the action is simple, if a valid source is not provided then the that field is omitted by the uploader. In the case of format there are a few paths we can take. (1) use a default, if format is omitted or nonsense then use the corresponding format from the default uploader map (2) omit the field from the upload string (3) use an arbitrary default eg %.3f (4) use a hybrid, eg use (1) but if the field does not exist in the default uploader map then use (3).

@tkeffer
Copy link
Contributor

tkeffer commented Oct 27, 2020

There's a pattern I've been using for situations like this. Take a look in accum.py. Basically it would look like:

DEFAULTS_INI = """
[StdRESTful]
  [[WOW]]
    [[uploader_map]]
        [[[[baromin]]]]
            source = barometer
            format = %.3f
        [[[[dewptf]]]]
            source = dewpoint
            format = %.1f
      ...
"""

Then, in the WOW uploader:

        merge_dict = get_site_dict(
            config_dict, 'WOW', 'station', 'password')
        if merge_dict is None:
            return
        
        _ambient_dict = ConfigObj(StringIO(DEFAULTS_INI), encoding='utf-8')['StdRESTful']['WOW']
        _ambient_dict.merge(merge_dict)

I'm sure I screwed something up. In particular, I'm not sure how the merge in the last line would react to being fed a subsection. Hopefully you get the idea.

  • This approach makes it easy to see and change the defaults.
  • It is a purely additive approach.
  • Options in weewx.conf always override the defaults.
  • To turn off an upload, the user could set source to None.
  • To change the source of an upload, the user just specifies a difference value for source. The old format would be used.
  • If a user specifies a bad format, it's on his/her head.

@gjr80
Copy link
Contributor Author

gjr80 commented Oct 27, 2020

Thanks, that makes sense.

@gjr80
Copy link
Contributor Author

gjr80 commented Oct 27, 2020

From my abbreviated testing this morning it seems to work as you describe. Did not strike any issue with the merge being fed a subsection, rather the problem is that get_site_dict() returns just scalars and no uploader_map section. There may be a better way but I just add uploader_map from config_dict (if it exists) to merge_dict and then do the merge and it works fine.

@tkeffer
Copy link
Contributor

tkeffer commented Oct 28, 2020

That looks good!

A few (trivial) suggestions:

  1. When assigning a function parameter to self, it's good practice not to change the name. So, instead of

     self.formats = uploader_map
    

    you want

     self.uploader_map = uploader_map
    

    Besides, it's not really a set of "formats."

  2. Instead of

     log.info("merge_dict=%s" % (merge_dict,))
    

    use

     log.info("merge_dict=%s", merge_dict)
    

    This way, processor time need not be spent on evaluating the string if logging is not enabled. See the docs for logging.

  3. You seem to be using an older version of restx.py.

@duane-nepa
Copy link

Have you considered storing this in a db table and using db tables for this metadata?

@tkeffer
Copy link
Contributor

tkeffer commented Oct 28, 2020

A fair question. Off the top of my head, here are some reasons:

  1. It's easier to modify a configuration file.
  2. All the state is held in the code and the configuration file, making it much easier to version.
  3. Support is easier when you don't have to ask a user to do a database query.
  4. These data can be deeply nested, which is difficult to capture in a flat SQL table.

@gjr80
Copy link
Contributor Author

gjr80 commented Oct 28, 2020

  1. When assigning a function parameter to self, it's good practice not to change the name. So, instead of
     self.formats = uploader_map
    

Think that was a hangover of the old and the new. Will fix.

  1. Instead of

     log.info("merge_dict=%s" % (merge_dict,))
    

    use

     log.info("merge_dict=%s", merge_dict)
    

    This way, processor time need not be spent on evaluating the string if logging is not enabled. See the docs for logging.

Thanks for pointing that out, I hadn’t noticed, still had the old syslog approach in my mind. Think I have a few (well a lot of) log statements to fix across numerous repos.

  1. You seem to be using an older version of restx.py.

Think this might be a timing issue, branched off development and committed before 4.2.0 merges occurred. Yesterday I merged in development and that picked up a pile of (4.2.0) commits but I may not have committed since. Will check.

@gjr80
Copy link
Contributor Author

gjr80 commented Oct 29, 2020

@tkeffer. Refactoring WU and I see that humidity is formatted using %03.0f whereas indoorhumidity is formatted with %.0f. I don't see anything in the WU upload protocol docs. Is this another WU vagary or can I just make them both %.0f.

@gjr80
Copy link
Contributor Author

gjr80 commented Oct 29, 2020

  1. You seem to be using an older version of restx.py.

Think this might be a timing issue, branched off development and committed before 4.2.0 merges occurred. Yesterday I merged in development and that picked up a pile of (4.2.0) commits but I may not have committed since. Will check.

Oh, and yes I hadn't pushed, should be up to date with development now.

@tkeffer
Copy link
Contributor

tkeffer commented Oct 29, 2020

There's a commit four years ago that explicitly addresses the formatting of indoor humidity: d29eba5

It seems to be there to address this email thread: https://groups.google.com/g/weewx-user/c/R71L5gLs1EM/m/kQFP005IAQAJ

It's possible we could send all numbers without zero padding, but I don't think we should mess with something that is working.

@gjr80
Copy link
Contributor Author

gjr80 commented Oct 29, 2020

There's a commit four years ago that explicitly addresses the formatting of indoor humidity: d29eba5

It seems to be there to address this email thread: https://groups.google.com/g/weewx-user/c/R71L5gLs1EM/m/kQFP005IAQAJ

It's possible we could send all numbers without zero padding, but I don't think we should mess with something that is working.

Another WU quirk/vagary, should have known. Will leave well enough alone.

@roe-dl
Copy link
Contributor

roe-dl commented May 23, 2021

The widely used MQTT uploader extension uses a slightly different mapping. It names the section by the WeeWX observation type name and uses an entry "name" to set the external field name. So it could be confusing to the user if for some uploaders the WeeWX name is the section and the external name is the config entry, and for other uploaders it is the opposite way. The superior section is called [[[inputs]]] for the MQTT extension.

Example of MQTT configuration snippet:

    [[MQTT]]
        ...
        [[[inputs]]]
            [[[[rain]]]]
                name = rain_mm
                units = mm
            ...

@gjr80
Copy link
Contributor Author

gjr80 commented May 24, 2021

Thanks, I need to get my head back around the RESTful uploaders but that won’t be until after 4.6.0 is released. I will keep this in mind.

Copy link

github-actions bot commented May 8, 2024

This issue is stale because it has been open for 180 days with no activity. It will be closed in 28 days.

@github-actions github-actions bot added the stale label May 8, 2024
Copy link

github-actions bot commented Jun 5, 2024

This issue was closed because it has been inactive for 28 days since being marked as stale.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants