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

Added support for setting data rate #19

Merged
merged 9 commits into from
Jul 1, 2020
Merged

Added support for setting data rate #19

merged 9 commits into from
Jul 1, 2020

Conversation

evaherrada
Copy link
Collaborator

@evaherrada evaherrada commented Jun 25, 2020

Tested with a feather M0 running CircuitPython 5.3.0

@evaherrada evaherrada requested a review from sommersoft June 25, 2020 19:47
@evaherrada
Copy link
Collaborator Author

Solves #12

@tannewt
Copy link
Member

tannewt commented Jun 26, 2020

I wouldn't encourage use of write_register because the goal of our drivers should be that they can be used without reading or understanding the datasheet. Instead, why not expose a new API for the specific thing someone was trying to achieve?

@evaherrada
Copy link
Collaborator Author

@tannewt Ok, I'll give that a go

@evaherrada
Copy link
Collaborator Author

@tannewt Ok, I allowed you to set the data rate when initializing it. I decided not to add a function to do that after initialization since the range was being set upon initialization as well.

@evaherrada evaherrada changed the title Better documentation in simpletest, new example: write_registers Added support for setting data rate Jun 29, 2020
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple things. Good overall!

Comment on lines 130 to 131
# self.write_register(_L3GD20_REGISTER_CTRL_REG1, 0x0F)
self.write_register(_L3GD20_REGISTER_CTRL_REG1, rate + 0x0F)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# self.write_register(_L3GD20_REGISTER_CTRL_REG1, 0x0F)
self.write_register(_L3GD20_REGISTER_CTRL_REG1, rate + 0x0F)
self.write_register(_L3GD20_REGISTER_CTRL_REG1, rate | 0x0F)

spi_busio,
cs,
rng=L3DS20_RANGE_250DPS,
rate=L3DS20_RATE_100HZ,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add rate after baudrate to prevent breaking code that relies on position of these args. It wouldn't matter if there was a *, before the kwargs to make them kwarg-only.

@evaherrada evaherrada requested a review from tannewt June 30, 2020 13:31
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! One more spot.

adafruit_l3gd20.py Outdated Show resolved Hide resolved
@evaherrada evaherrada requested a review from tannewt June 30, 2020 18:17
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you!

@tannewt tannewt merged commit 63af6cf into adafruit:master Jul 1, 2020
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jul 3, 2020
Updating https://github.com/adafruit/Adafruit_CircuitPython_BH1750 to 1.0.1 from 1.0.0:
  > Update README.rst

Updating https://github.com/adafruit/Adafruit_CircuitPython_L3GD20 to 2.3.0 from 2.2.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_L3GD20#19 from dherrada/master

Updating https://github.com/adafruit/Adafruit_CircuitPython_Display_Text to 2.7.6 from 2.7.5:
  > Merge pull request adafruit/Adafruit_CircuitPython_Display_Text#57 from FoamyGuy/fix_anchor_when_text_is_scaled
@evaherrada evaherrada linked an issue Jul 6, 2020 that may be closed by this pull request
@evaherrada evaherrada mentioned this pull request Jul 6, 2020
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.

Expand Examples
3 participants