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

Consistency in unified sensor driver property naming #622

Closed
tuupola opened this issue Feb 13, 2018 · 16 comments
Closed

Consistency in unified sensor driver property naming #622

tuupola opened this issue Feb 13, 2018 · 16 comments
Assignees
Milestone

Comments

@tuupola
Copy link

tuupola commented Feb 13, 2018

To avoid hijacking an unrelated thread I move this here. Currently there is mixed capitalization in the design guide concerning unified sensor driver property naming.

Consistency is usually good thing. If everything is lowercased there is no need to remember whether one should use capital letters or not. For example lux might be confusing. Is it acronym or word? Should it be sensor.lux or sensor.LUX? Also when seeing sensor.TVOC one is not really sure if it is a constant or property.

I do not have strong feelings about this, but my choice would be lowercased acronyms sensor.tvoc and sensor.eco2.

@tannewt
Copy link
Member

tannewt commented Feb 15, 2018

I've been silent on this because I haven't felt super strong about it. I thought about expanding them out (total_volatile_organic_compound for example) but then they get really long. Ultimately, I agree with you @tuupola. Switching to lowercase makes sense to me.

@tannewt tannewt added this to the 3.0 milestone Feb 28, 2018
@tannewt
Copy link
Member

tannewt commented Feb 28, 2018

@tuupola Would you like to switch them to the lower case version?

@brentru
Copy link
Member

brentru commented Mar 5, 2018

These two libs to be changed to reflect the move to lowercase (am I forgetting any others which use the TVOC/eCO2 property naming?):
https://github.com/adafruit/Adafruit_CircuitPython_SGP30
https://github.com/adafruit/Adafruit_CircuitPython_CCS811

@tuupola
Copy link
Author

tuupola commented Mar 5, 2018

Had missed this. @tannewt sure, I will check them out.

@tannewt
Copy link
Member

tannewt commented Mar 5, 2018 via email

@evaherrada
Copy link

evaherrada commented May 14, 2018

@tuupola I put in a pull request fixing those issues to the CCS811 library and it was accepted. I didn't see any problems with the SGP30 library.

@tannewt
Copy link
Member

tannewt commented May 16, 2018

SGP30 is missing properties all together. They should be added.

@tannewt tannewt self-assigned this May 24, 2018
@tannewt
Copy link
Member

tannewt commented May 25, 2018

@mrmcwethy would you be interested in adding eco2 and tvoc properties to the SGP30 driver?

@tannewt tannewt removed their assignment May 25, 2018
@mrmcwethy
Copy link
Collaborator

@tannewt I would be happy to add these properies. I will need to order a sgp30 board.

@tannewt
Copy link
Member

tannewt commented May 29, 2018

Go ahead and order one (and other other sensors of interest). I think @brentru may have started the sgp30 work though.

@brentru
Copy link
Member

brentru commented May 29, 2018

Working through the SGP30 right now.

@tannewt Do we want properties for the baseline sensor readings (would be the same as the call in the example: https://github.com/brentru/Adafruit_CircuitPython_SGP30/blob/master/examples/sgp30_simpletest.py#L27) as well? Naming would be: tvoc, tvoc_base, co2eq, co2eq_Base

@tannewt
Copy link
Member

tannewt commented May 29, 2018

Yes please. I'd name them baseline_tvoc and baseline_co2eq though. Thanks!

@tannewt
Copy link
Member

tannewt commented Jun 1, 2018

@brentru
Copy link
Member

brentru commented Jun 3, 2018

@tannewt will do.

@brentru brentru self-assigned this Jun 3, 2018
@brentru
Copy link
Member

brentru commented Jun 4, 2018

Modified the CCS and SGP30 guides to match the naming consistencies.

@tannewt
Copy link
Member

tannewt commented Jun 4, 2018

Awesome! I think this is done then. Please reopen if not.

@tannewt tannewt closed this as completed Jun 4, 2018
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

5 participants