adafruit / Adafruit_CircuitPython_BusDevice

Two helper classes that handle transaction related state for I2C and SPI including locks.
MIT License
108 stars 76 forks source link

Enable CS "active-high" device support to resolve #71 #72

Closed michthom closed 3 years ago

michthom commented 3 years ago

Reference https://github.com/adafruit/Adafruit_CircuitPython_BusDevice/issues/71 Enables SPIDevice to be used for devices which requires CS to be active high. Adds a new boolean parameter cs_active_value. Linked PR raised against circuitpython shared bindings / shared module code.

michthom commented 3 years ago

@gamblor21 Pass the dunce cap - I thought I'd checked these but a bug has crept back in - now fixed. Missing comma reinserted. Local checks with black passing again. And now with a line too long (comment) fixed as well.

michthom commented 3 years ago

@gamblor21 Locally, I'm seeing pylint failures only for the examples (f-string recommendations for formatting). Would you like me to include those in this patch, to get it clean? Or a separate PR as that's not anything I've touched?

gamblor21 commented 3 years ago

@gamblor21 Locally, I'm seeing pylint failures only for the examples (f-string recommendations for formatting). Would you like me to include those in this patch, to get it clean? Or a separate PR as that's not anything I've touched?

edit sorry I missed that this was causing the checks to fail. If you can include the changes in this PR that would work.

michthom commented 3 years ago

@gamblor21 I've included the pylint f-string fix here, so both my PR should (hopefully) now be ready for review / merge. Thanks for your help.

michthom commented 3 years ago

@gamblor21 - I'm baffled, sorry. The proposed fix to the print issue is now itself failing pylint checks with undefined variable. Could this be a variant of https://github.com/PyCQA/pylint/issues/3791 or have I messed up - I tried the code in the REPL and all seemed well... but pre-commit fails. I replaced print("".join("{:02x}".format(x) for x in result)) with print("".join([f"{x:02x} for x in result"])) and pre-commit fails with (e.g.) examples/busdevice_read_register_i2c_simpletest.py:20:16: E0602: Undefined variable 'x' (undefined-variable)

michthom commented 3 years ago

I've defined x="" in both example files to silence pylint's error. Ugly, so happy to change if there is a more elegant fix? But at least the pre-commit is passing now.

michthom commented 3 years ago

@gamblor21 This is ready for a new build test now.

gamblor21 commented 3 years ago

So a couple comments after checking with others who know pylint and python much better then I:

  1. Can you undo the example changes back to how they were?
  2. Instead can you modify .pre-commit-config.yaml and add to the end consider-using-f-string to the list of warnings ignored. The line would be: args: ['([[ ! -d "examples" ]] || for example in $(find . -path "./examples/*.py"); do pylint --disable=missing-docstring,invalid-name,consider-using-f-string $example; done)']

See: https://github.com/adafruit/Adafruit_CircuitPython_DS18X20/blob/main/.pre-commit-config.yaml for an example that someone else did when they ran into the same problem.

If any of this gets confusing please let me know and I can help as well. And thank you so much for your patience with this PR!

michthom commented 3 years ago

Certainly - that seems like the elegant solution (until pylint can grok comprehensions inside other structures). I'll get that lined up and hopefully it'll all line up with the esp-idf cache key fix too.

michthom commented 3 years ago

@gamblor21 - all done. I'm happy to help, I will be the beneficiary in the long run!

gamblor21 commented 3 years ago

@gamblor21 - all done. I'm happy to help, I will be the beneficiary in the long run!

One hopefully last thing, can you undo the example changes, the additional ignore flag should remove the need for that.

michthom commented 3 years ago

@gamblor21 Sure - I misunderstood and thought you only wanted the ugly x="" gone. I'll revert both.