search

adafruit / Adafruit_CircuitPython_MotorKit

CircuitPython helper library for the DC & Stepper Motor FeatherWing, Shield and Pi Hat kits.
MIT License
86 stars 31 forks source link

Make "i2c" a required first parameter #25

Closed geekguy-wy closed 4 years ago

geekguy-wy commented 4 years ago

Currently, we have to include "i2c=i2c" when we instantiate a MotorKit. This is inconsistent with how other libraries work with "i2c" as a required first parameter. This is an easy fix to bring MotorKit in line with the rest of the libraries. I realize this will break code, but it needs to be done so we have consistency between libraries.

ladyada commented 4 years ago

tagging @kattni since kattni wrote the library and may have a reason for doing it that way

kattni commented 4 years ago

@geekguy-wy I don't understand. None of the code requires you to include i2c=i2c on instantiation. I haven't tested it recently - are you suggesting that all of the examples currently failing and incorrect? Please clarify what you are referring to.

siddacious commented 4 years ago

@geekguy-wy this isn't a "library" in the sense that Adafruit_CircuitPython_LIS3DH etc. are libraries; this would be more succinctly called a "helper library" as opposed to a "driver library". It is aimed at making it easy to work with by having sensible defaults, setting up objects for you, and providing high level methods and properties. It's in the same class as Adafruit_CircuitPython_CLUE and Adafruit_CircuitPython_CircuitPlayground

geekguy-wy commented 4 years ago

When I started using the MotorKit library, I had to use "drive = MotorKit(i2c = i2c)" to instantiate a MotorKit object. This is not consistent with the way I have been doing things with other libraries. I have only done "aa = library_name(i2c, address=0xNN)" where "i2c" is required as the first parameter and is not a keyword argument. The current syntax is not consistent with other libraries.

geekguy-wy commented 4 years ago

I understand about it being a helper and all that. However, the syntax and the way it is instantiated should remain consistent throughout all libraries and helpers if they are accessible by users.

siddacious commented 4 years ago

@geekguy-wy the i2c kwarg is not required: image (it's failing because I don't have a PCA connected but I2C works just fine)

This is consistent with the other similar helper libraries I mentioned above.

I disagree with your assertion that all libraries must have the same init signature. It's an intentional choice that we've made for the helper libraries to work with sane defaults. If you want to use something that works the way you're used to, you can use the Adafruit_CircuitPython_PCA9685 library directly. I'm locking this since I think we understand each other though we don't agree.

tannewt commented 4 years ago

@geekguy-wy I understand the inconsistency you are seeing and it would be nice if the kwarg order matched how it is when i2c is required.

However, as @siddacious pointed out, we do have two tiers of drivers. Ones that are low-level IC based where we make no assumptions about the world around it and ones that are all-in-one, assume everything and init it all for you. The latter is where the FeatherWing libraries and the *Kit libraries live. Hopefully they can be further sample code one can hack to achieve what is desired.