new driver for air quality monitor device

SoftwareDefinedBuildings / smap

smap - Simple Measurement and Actuation Profile

Other

30 stars 36 forks source link

new driver for air quality monitor device #7

Closed anthonysutardja closed 10 years ago

anthonysutardja commented 10 years ago

Let me know if I'm doing this right

stevedh commented 10 years ago

Generally looks good. Thanks for the PR. Main things I notice are

probably shouldn't be in the thermostat package since it's not a thermostat
in the future you could subclass smap.driver.FetchDriver for this sort of thing. Right now there's not much point since it also uses urllib (rather than t.w.c.Agent) but hey maybe we'll fix that.

andrewfang commented 10 years ago

The device has a thermostat built on, so we're pulling that data as well as the humidity and gas pressure measurements. Should we make a new class of devices?

immesys commented 10 years ago

I think steve is saying that although it has a thermometer, it is not a thermostat - i.e it is not designed to control an HVAC system. You can just copy it one directory up or create a new directory.

stevedh commented 10 years ago

Yep.

immesys commented 10 years ago

I would prefer if the error conditions were less silent. Can you add some logging to things like the ValueError for instance? If they are expected (I have a device that frequently issues invalid data for example) then please add a comment to that effect. Otherwise silently absorbing exceptions is a bit mean for future developers.

Apart from that, LGTM

anthonysutardja commented 10 years ago

Sounds good, I'll move it up a dir. Should errors just be a print statement?

immesys commented 10 years ago

Steve can weigh in on what the 'correct' logging facility in sMAP is, but I think Twisted has a logging API

stevedh commented 10 years ago

Don't know if you're waiting for signoff here, ship it!