Skip to content

Add typing, small refactoring #10

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

Merged
merged 9 commits into from
Jan 10, 2022
Merged

Add typing, small refactoring #10

merged 9 commits into from
Jan 10, 2022

Conversation

tekktrik
Copy link
Member

@tekktrik tekktrik commented Nov 17, 2021

Addresses Issue #9 but found some minor improvements that would make typing more clear as well as remove what seemed like unused parts of the code. The only change from the perspective of function input and output is that add_values() now only needs two arguments, but that function is run by the module on import, not a user.

If its a good idea, I can try and do some more refactoring, but I didn't want to change too much for the scope of this pull request.

I think there's more room for improvement but I didn't want to make any drastic changes
@tannewt tannewt requested a review from a team November 17, 2021 18:17
@tekktrik
Copy link
Member Author

After working on some other libraries, I see the purpose of the string class attribute of CV. I've added it back in, but removed the lsb attribute because it still serves no purpose in this specific case.

@tekktrik
Copy link
Member Author

To help clear that misunderstanding up, as well as provide an example of it's use, I changed it from Rate.string to Rate.label because to me it implied it's type would be str. I also added it's usage to the example. Let me know if I should change or revert any of this.

@tekktrik
Copy link
Member Author

If the change from string to label makes sense, it may be worth considering doing it for the other libraries that implement their own CV classes, there are a few of them.

("ONE_SHOT", 0, 0, None),
("RATE_1_HZ", 1, 1, None),
("RATE_7_HZ", 2, 7, None),
("RATE_12_5_HZ", 3, 12.5, None),
Copy link
Contributor

Choose a reason for hiding this comment

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

Did these Nones serve any purpose previously?

Copy link
Member Author

Choose a reason for hiding this comment

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

My understanding from other libraries that do this is the store the resolution or LSB. As an example, if an option changes the sensitivity of a sensor to the equivalent of 8 bits, this would be 0.0039. So it's just that it doesn't here because it changes rate, not anything where the value of a "unit" of it changes, if that makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you run it with a device? Seems okay to me. But want to try to make sure it won't cause IndexError anywhere ifi we can. I don't have one of these sensors.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think I did, I don't think I own this sensor. The other libraries use it in calculations (calculating temperature using the LSB value and digital value, like from ADC) but it never seemed to be used in communication with the sensor. Happy to find a sensor and test with it if you'd like!

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out the HTS221 is out of stock on Adafruit and Digi-Key with no restock date :/ depreciated maybe?

Copy link
Contributor

@Neradoc Neradoc left a comment

Choose a reason for hiding this comment

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

The simple test gives:

Traceback (most recent call last):
  File "code.py", line 10, in <module>
  File "adafruit_hts221.py", line 189, in __init__
  File "adafruit_hts221.py", line 269, in data_rate
AttributeError: data_rate must be a `Rate`

I put fixes in the review.

@tekktrik
Copy link
Member Author

tekktrik commented Jan 7, 2022

Changes made, thank you so much!

@Neradoc
Copy link
Contributor

Neradoc commented Jan 9, 2022

Seems all good now, no issue in my tests.

Copy link
Contributor

@FoamyGuy FoamyGuy 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 to me. Thank you @tekktrik and thank you to @Neradoc for trying it out.

@FoamyGuy FoamyGuy merged commit 425f070 into adafruit:main Jan 10, 2022
@FoamyGuy FoamyGuy mentioned this pull request Jan 10, 2022
4 tasks
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jan 12, 2022
Updating https://github.com./adafruit/Adafruit_CircuitPython_DRV2605 to 1.2.0 from 1.1.13:
  > Merge pull request adafruit/Adafruit_CircuitPython_DRV2605#29 from tekktrik/doc/typing-and-documentation
  > Merge pull request adafruit/Adafruit_CircuitPython_DRV2605#28 from tekktrik/feature/add-eight-slot
  > update rtd py version

Updating https://github.com./adafruit/Adafruit_CircuitPython_HTS221 to 1.1.7 from 1.1.6:
  > Merge pull request adafruit/Adafruit_CircuitPython_HTS221#10 from tekktrik/feature/add-typing
  > update rtd py version

Updating https://github.com./adafruit/Adafruit_CircuitPython_PCA9685 to 3.4.0 from 3.3.9:
  > Merge pull request adafruit/Adafruit_CircuitPython_PCA9685#42 from tekktrik/doc/add-typing
  > Merge pull request adafruit/Adafruit_CircuitPython_PCA9685#43 from tekktrik/feature/add-mode2-reg
  > update rtd py version

Updating https://github.com./adafruit/Adafruit_CircuitPython_Wiznet5k to 1.12.0 from 1.11.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_Wiznet5k#49 from AdamCummick/main

Updating https://github.com./adafruit/Adafruit_CircuitPython_AVRprog to 1.4.2 from 1.4.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_AVRprog#31 from aarontusko/fix-write_fuses-busy_wait

Updating https://github.com./adafruit/Adafruit_CircuitPython_OneWire to 1.2.8 from 1.2.7:
  > Merge pull request adafruit/Adafruit_CircuitPython_OneWire#24 from tekktrik/doc/add-typing
  > update rtd py version

Updating https://github.com./adafruit/Adafruit_CircuitPython_PIOASM to 0.5.4 from 0.5.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_PIOASM#34 from dannystaple/mov_operators

Updating https://github.com./adafruit/Adafruit_CircuitPython_Requests to 1.10.5 from 1.10.4:
  > Merge pull request adafruit/Adafruit_CircuitPython_Requests#94 from kevincon/fix-93

Updating https://github.com./adafruit/Adafruit_CircuitPython_Waveform to 1.3.9 from 1.3.8:
  > update rtd py version
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