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

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

Merged
merged 1 commit into from Sep 23, 2021

Conversation

michthom
Copy link
Contributor

Reference #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.

Copy link
Member

@gamblor21 gamblor21 left a comment

Choose a reason for hiding this comment

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

Changes look good here but I have not had a chance to test it yet. Approved the workflow to run to make sure that is okay.

@michthom
Copy link
Contributor Author

michthom commented Sep 22, 2021

@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 michthom force-pushed the michthom-patch-1 branch 2 times, most recently from 0d55b6d to 3259adb Compare September 22, 2021 23:17
@michthom
Copy link
Contributor Author

michthom commented Sep 22, 2021

@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
Copy link
Member

gamblor21 commented Sep 23, 2021

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

michthom commented Sep 23, 2021

@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 pylint-dev/pylint#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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@gamblor21
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@gamblor21
Copy link
Member

@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
Copy link
Contributor Author

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

Reference adafruit#71

Enables SPIDevice to be used for things like the Sitronix ST7920 LCD display which requires CS to be pulled high during commands or data transfers.

Adds a new attribute (cs_active_value) to set the preferred logic for the CS line. Default false (active low).
@gamblor21 gamblor21 merged commit 11abbd8 into adafruit:main Sep 23, 2021
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Sep 25, 2021
Updating https://github.com/adafruit/Adafruit_CircuitPython_DS18X20 to 1.3.7 from 1.3.6:
  > Merge pull request adafruit/Adafruit_CircuitPython_DS18X20#23 from caternuson/new_example
  > Moved default branch to main
  > Moved CI to Python 3.7
  > Added help text and problem matcher
  > Added pull request template

Updating https://github.com/adafruit/Adafruit_CircuitPython_IS31FL3741 to 1.1.2 from 1.1.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_IS31FL3741#6 from adafruit/philb-clip-fix

Updating https://github.com/adafruit/Adafruit_CircuitPython_BusDevice to 5.1.0 from 5.0.6:
  > Merge pull request adafruit/Adafruit_CircuitPython_BusDevice#72 from michthom/michthom-patch-1
  > Merge pull request adafruit/Adafruit_CircuitPython_BusDevice#68 from 2bndy5/note-about-builtin-pkg
  > Moved default branch to main
  > Moved CI to Python 3.7
  > Added help text and problem matcher
  > Added pull request template
  > "Increase duplicate code check threshold "
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.

None yet

2 participants