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

Add dict interface #16

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

electronsandstuff
Copy link

Hey folks, for my application I added some methods that provide aggregate output from the data class. The added methods are:

  • get_all_pm - Returns all PM measurements as a list of dicts that contain the values and tags associated with measurements
  • get_all_counts - Returns the particle counts as a dict where the key is particle size
  • as_influxdb_line_proto - Represents the data as a string in the format of influxdb line protocol.
  • __iter__ - allows the class to be converted into a dict by calling dict(data). This may help resolve what is asked for in issue Seperate Readouts #15

These are useful for my use case of calling this package and outputting the data into an influxdb database using telegraf. I was able to test everything on my raspberry pi and I am receiving data. I thought that I'd offer the changes here if you think they should be part of the package.

@Gadgetoid
Copy link
Member

I'm intrigued by outputting data in influxdb format- I'm mired in the low level writing of drivers and I don't get much exposure to how people actually use these, but I head about influxdb a lot and it seems like sensors across the board could benefit from this kind of endpoint.

Whether it deserves to be in the driver or not is debatable. You never know how many other database systems or logging methods people might want supported, and it could end up being messy and complicated to endorse them all.

Excuse the tests exploding, I need to fix those!

@Gadgetoid
Copy link
Member

Hokay I have fixed the tests, could I be a pain and ask you to rebase, please?

@electronsandstuff
Copy link
Author

electronsandstuff commented Feb 7, 2022

OK, just rebased.

And yeah, whether or not the influxdb output belongs in the driver crossed my mind too and I had a couple of thoughts on it:

  • The line protocol is just a text representation of the data class and not a full on connection to a database. The format is also used for applications other than influxdb. This made me feel more OK about including it since it's more similar in my mind to another version of __repr__ or to numpy's tolist() type of functions than a DB connector. Users could even parse the formatted text for other applications.
  • It seemed more practical for the code to be shared here rather than distribute it separately. It's only four lines and so on it's own it didn't make sense to throw in a new package. Putting it in a gist could work, but then it's just another step for users to have to copy it down and get it running versus just installing this package and having it available right there.
  • In my (admittedly small) part of the internet, influxdb is the thing that people are using to store the data from this particular sensor and so I feel there's an audience for this feature.

It's still not a perfect fit, but it did feel like something that would save people time and effort.

@coveralls
Copy link

coveralls commented Feb 8, 2022

Pull Request Test Coverage Report for Build 1816522940

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 17 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+4.6%) to 76.423%

Files with Coverage Reduction New Missed Lines %
pms5003/init.py 17 76.42%
Totals Coverage Status
Change from base Build 1806616211: 4.6%
Covered Lines: 94
Relevant Lines: 123

💛 - Coveralls

@Gadgetoid
Copy link
Member

influxdb is the one solution I've actually heard of- short of pushing stuff right into a cloud service- so I'm inclined to believe its popularity extends past your neck of the woods. It's a good idea and it's great to have feedback and input from someone actually using this library.

Couple of minor issues:

  1. time.time_ns isn't available in Python <= 3.6 (it was introduced in 3.7). I'm going to hazard a guess that you don't really need 1/1000000000th of a second precision and would suggest using milliseconds - ie: time.time() * 1000 here.
  2. We're still soft supporting Python 2.7 though I'm slowly phasing that out, so the f-string explodes that test. Usually I'd move onwards and upwards but since this is just in the new method, you could probably use the spread operator like:
ef as_influxdb_line_proto(meas_name='pms5003', timestamp=True):
    """
    Get the data in the form of influxDB line protocol
    :return: str: the formatted data
    """
    ret = ["{name},size={size},environment={environment} pm={val}u".format(name=meas_name, **x) for x in self.get_all_pm()]
    ret.extend(["{},size={} count={}u".format(meas_name, s, c) for s, c in self.get_all_counts().items()])
    if timestamp:
        ret = ["{} {}".format(x, self.timestamp) for x in ret]
    return '\n'.join(ret)

@electronsandstuff
Copy link
Author

Ahhh, tripped up on python versions.

I think those changes should be good and just added them to the branch. For the timestamp, I switched it to float time in seconds, then convert to integer nanoseconds for influxdb later.

I'll install the latest version on my raspberry pi when I get home tonight just to make sure I didn't make any dumb mistakes. However, it's such a small change that I expect everything should be good to go.

@electronsandstuff
Copy link
Author

OK, the code is on my raspberry pi and everything seems to be working. I just uploaded the script I use to the examples directory too.

@Gadgetoid Gadgetoid mentioned this pull request Jan 20, 2023
@Gadgetoid
Copy link
Member

Okay just to be completely turbo annoying, I am migrating this library over to gpiod since the writing is on the wall for RPi.GPIO whether it survives the Pi 5 transition or not. See #19

Python 2.x support will be dropped so we can go nuts with the fancy stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants