adafruit / Adafruit_CircuitPython_APDS9960

Adafruit Bundle driver for APSD9960 Gesture breakout board
MIT License
11 stars 17 forks source link

Property-fy proximity, add distance? #18

Closed caternuson closed 4 years ago

caternuson commented 4 years ago

The proximity value is currently accessed via a function call:

apds.proximity()

It would be more inline with other libraries to make that a property.

apds.proximity

Also - not seeing any entry in the Design Guide for proximity sensor: https://circuitpython.readthedocs.io/en/latest/docs/design_guide.html#sensor-properties-and-units Closest is distance, which may be possible in an approximate way by curve fitting one or more of these (from datasheet): image

ladyada commented 4 years ago

im ok with a proximity property - distance really means distance, which this is not guaranteed to be :)

tannewt commented 4 years ago

It's totally ok to add a new entry to the design guide. The source is here: https://github.com/adafruit/circuitpython/blob/master/docs/design_guide.rst#sensor-properties-and-units

We should add sound_level too.

kevinjwalters commented 4 years ago

Are you doing this for consistency reasons or because its "Pythonic?".

I was trying to work out the other day a rule for deciding on a method return value vs property. If there's no historical use of an attribute and no setter and no intention/possibility of a setter then I can't see the value in a property?

caternuson commented 4 years ago

For CircuitPython, it's part of the Design Guide: https://circuitpython.readthedocs.io/en/latest/docs/design_guide.html#getters-setters

kevinjwalters commented 4 years ago

BTW, what's the unit for this gyro property?

>>> from adafruit_clue import clue
>>> clue.gyro
(2.89625, -9.19625, -4.94375)

The design guide has units for gyro as x, y, z radians per second but my guess was this was degrees per second? I may be wrong there...

kevinjwalters commented 4 years ago

Perhaps everyone's already aware of this one but I see readPressure() in https://github.com/adafruit/Adafruit_Arcada/blob/master/examples/full_board_tests/arcada_clue_sensorplotter/arcada_clue_sensorplotter.ino#L119 being plotted as Pa (Pascals) and not hPa as per https://learn.adafruit.com/using-the-adafruit-unified-sensor-driver/how-does-it-work#standardised-si-units-for-sensor-data-2-2

ladyada commented 4 years ago

@kevinjwalters please open an issue in the arcada repo if you see a bug!

ladyada commented 4 years ago

gyros are radians per second

caternuson commented 4 years ago

For CircuitPython, property names and units are documented here: https://circuitpython.readthedocs.io/en/latest/docs/design_guide.html#sensor-properties-and-units

kevinjwalters commented 4 years ago

I'll see if my 50s record player works later for an rpm test...

kevinjwalters commented 4 years ago

I get around 220 somethings per second at 45rpm from CP's gyro on an HMV record player which needs a service. I put in https://github.com/adafruit/Adafruit_CircuitPython_LSM6DS/issues/9 for this.

kevinjwalters commented 4 years ago

For C/Arduino library for BMP280 sensor, someone has already noted discrepancy based on a comment in code, i'd added detail to the closed issue for this one: https://github.com/adafruit/Adafruit_BMP280_Library/issues/35#issuecomment-596030828

tannewt commented 4 years ago

Are you doing this for consistency reasons or because its "Pythonic?".

I was trying to work out the other day a rule for deciding on a method return value vs property. If there's no historical use of an attribute and no setter and no intention/possibility of a setter then I can't see the value in a property?

Properties themselves are more Pythonic. They represent state that exists regardless of whether it is being read. Functions are sequences of actions.

The design guide includes specific attribute names and units so that drivers can be used interchangeably.

caternuson commented 4 years ago

So....we OK with making this a breaking change? Not sure if there is a way to support both syntaxes?

kevinjwalters commented 4 years ago

You'll need to do a synchronised update to any libraries using this, e.g. at least:

tannewt commented 4 years ago

@caternuson Ya, I'm ok breaking it. Just make sure to do a major version bump and ideally change all uses like clue close to the same time.

caternuson commented 4 years ago

Done with #19