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

PEP8 compliance for pyRF24 package #1

Closed
7 tasks done
2bndy5 opened this issue Sep 21, 2020 · 11 comments · Fixed by #5
Closed
7 tasks done

PEP8 compliance for pyRF24 package #1

2bndy5 opened this issue Sep 21, 2020 · 11 comments · Fixed by #5

Comments

@2bndy5
Copy link
Member

2bndy5 commented Sep 21, 2020

I started talking about the pyRF24 examples in relation to poor formatting (mostly readability) in nRF24/RF24#391, but that quickly got off topic. This issue will be my home for updating pyRF24 package as I have some concerns:

  1. The name pyRF24 is ok as a folder name, but pypi.org expects package names to be all lowercase using - as word delimiters. Furthermore, after installing of pyRF24, the wrapper is registered under the package name RF24 -- this is not ok. Package names should be as descriptive as possible and use the same convention pypi.org expects, except _ in place of - since - would be interpreted as a subtraction in python import statements. Not to mention the RF24 package can be confused with the RF24 class especially for namespace-ing reasons. On that note, from RF24 import * is discouraged as risky. Tagging [Question] Distributing Python wrapper module with pip RF24#589 as this issue progresses that one.
  2. The API for C++ does not conform to PEP8 standards. There are a few required changes to make it so:
    • Function, attribute, parameter, and module names should follow snake-casing convention. Class names are camel-casing. Constants should be in all caps using _ as word delimiters. Currently pyRF24's classes & constants are PEP8 compliant. Technically the module names don't apply to pyRF24 because of the nature of wrapping C++. Function, parameter, & attributes names can be changed due to the nature of wrapping C++ with libboost-python. See PEP8 Naming Convention guidelines.
    • Python decorated attributes aren't really being taken advantage of in pyRF24. With the use of python's builtin decorators, setters and getters should be consolidated into decorated attributes. This is because python does not protect against user access to private members, thus the naming convention for "magic functions" (e.g. __repr__()) and private members (e.g. _private_member). However, libboost-python abstracts the "magic methods" for us, and Arduino convention recommends the same prefix for private members. This one isn't listed as part of the PEP8 convention, but it is a VERY COMMON practice in python.
    • Whitespace and line length can be easily addressed with the python package called black. Pylint is more user friendly as it can flag errors in VSCode as well as make suggestions about optimizing readability (e.g. using for i, val in enumerate(my_list): instead of for i in range(len(my_list)):)
    • Docstrings must be included for different scopes. Module-level and function-level docstrings are the only scopes that really apply to pyRF24 (in examples). Docstrings in python are triple quoted strings of text.
    • Certain keywords already defined for python should be avoided. Most important example of this that I've noticed is when creating references for parameters in the wrapper and declaring variables in the examples: The use of len to describe the length of a buffer; len in python is a builtin function that returns the length of an iterable object.
    • Python packages are structured to allow multiple modules under the same namespace. The way the wrapper is written currently allows 1 module per RF24 library. Meaning pyRF24 installs as RF24 package containing a RF24 class; pyRf24Mesh installs as RF24Mesh package containing a Rf24Mesh class; pyRF24Network installs as RF24Network package containing a RF24Network class. Since these RF24* libraries' dependency is incremental (RF24 <- RF24Network <- RF24Mesh), they should be installed as 3 different modules in 1 package. This would also simplify namespacing for python coders as well.
    • Python2 has reached End-of-Life as of January 2020. Debian-based distros will continue to ship with python2 only so that python2 code will still run, but python2 developers have had ample time and notice to migrate to python3. I say this because libboost-python has dropped python2 support as of v1.70 (as of this writing v1.67 is the version that apt will install). To address this, we only need to remove install instructions for python2 from the docs if/when python2 stops shipping with debian-based distros or if/when libboost-python v1.70 becomes the default version installed by apt. The setup.py files should also be adjusted at the same time.
@Avamander
Copy link
Member

If you start fixing, use a Pull Request for it first, that way it can be reviewed.

@2bndy5
Copy link
Member Author

2bndy5 commented Sep 21, 2020

I'd also like to acknowledge why CircuitPython support is not possible with RF24* libraries.

  1. CircuitPython is a firmware. adafruit-blinka is a pypi package that "backports" CircuitPython API into CPython on linux-based SoC boards. Adafruit-PureIO is listed as a dependency in adafruit-blinka requirements.txt as it implements access to the SPI bus to expose a pythonic interface similar to the spi-dev python wrapper. CircuitPython does not support importing python extensions written in C or C++.
  2. I have already started a pypi package for the nRF24L01 to be used in the CircuitPython firmware, which can also be used on linux based SoC boards. This package occupies the proper naming convention for CircuitPython packages on pypi.

@Avamander
Copy link
Member

If not CircuitPython, what about just MicroPython? Still not possible?

@2bndy5
Copy link
Member Author

2bndy5 commented Sep 21, 2020

@Avamander nRF24/RF24#593

@2bndy5
Copy link
Member Author

2bndy5 commented Sep 21, 2020

it requires a separate wrapping implementation, and older micropython devices (like esp8266 and micro-bit's nRF52822) have been struggling with memory allocation errors because of the size of the firmware.

@2bndy5
Copy link
Member Author

2bndy5 commented Sep 21, 2020

@Avamander actually i was thinking a separate branch to merge PRs to, then merge to master when its ready. Additionally, this is a huge undertaking, and I am humbly not fully equipped to do it alone.

@Avamander
Copy link
Member

@Avamander actually i was thinking a separate branch to merge PRs to, then merge to master when its ready. Additionally, this is a huge undertaking, and I am humbly not fully equipped to do it alone.

A PR functionally already is a separate branch.

@2bndy5
Copy link
Member Author

2bndy5 commented Sep 21, 2020

oops I was thinking in terms of forks

@2bndy5
Copy link
Member Author

2bndy5 commented Sep 21, 2020

Consider this when changing the install instructions of python wrapper in docs.

I ran into a pip cannot uninstall <package> because it was installed using distutils... For me the package in question was RF24Network. Aparently this is from using python3 setup.py build & python3 setup.py install pip thinks it was installed using apt and refuses to uninstall because it doesn't know where all the files are located (metadeta about installed files was missing). I had to uninstall manually using this answer on stackoverflow. Note that the answer recommends using pip3 install . which builds and installs properly (from my experience).

Switching to virtual environments during testing.

@2bndy5
Copy link
Member Author

2bndy5 commented Sep 25, 2020

pyRF24Network module seems to want to inject itself into the namespace for the pyRF24 module using boost-python's scope(). This is because the modules are installed as separate python packages. So far I've managed to PEP8-ify the pyRF24 module...

The main problem here is that you need to git clone all RF24* repos then configure make for each (don't need to actually make or make install due to setup.py using distutils.unixccompiler & distutils.ccompiler modules). This workflow results in 3 single module packages where it should be 1 package with 3 modules. The 1 package solution wouldn't have to require boost-python's scope() (which doesn't seem officially supported anymore -- can't find it in vastness of boost-python's latest docs).

Maybe I could get setup.py to git clone all 3 RF24* repos and automate the compiling separately from this repo's context, but I think the python wrapper should go in its own repo for that (kinda like the RF24 libs docs are hosted by a separate repo). TBH I'd rather devote more time to my pure python porting of RF24Network & RF24Mesh into 1 CircuitPython package that can be used on any platform this python wrapper can be used.

p.s. I took a quick peek at pyRF24Mesh wrapper, and it doesn't use boost-python's scope() to inject itself into another package's namespace. Don't exactly know why pyRF24Network is.

@2bndy5
Copy link
Member Author

2bndy5 commented Nov 2, 2020

A note about using boost.python

the following error occured during testing a new python example:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/pi/RF24/examples_linux/manual_acknowledgements.py", line 70, in master
    radio.read(ack, radio.getDynamicPayloadSize())
Boost.Python.ArgumentError: Python argument types in
    None.read(RF24, bytes, int)
did not match C++ signature:
    read(RF24 {lvalue}, int maxlen)

It would seem that boost.python expects all arguments to be lvalues. So, passing a function call to use its rvalue as an parameter doesn't get evaluated like with normal python.

p.s. I ran into a similar problem (no traceback error) when passing a bytes object from a list of buffers (all bytes objects) to writeFast().

EDIT

I just realized that the read() call was being used improperly (in python wrapper). Coming from a python background, this library is rather painful to code for in python. Especially when the only docs applicable to the python wrapper are written for Arduino-based C++. I'm doing my best to note discrepancies in the docs (as I find them "the hard way"), but that doesn't make me better about it.

@2bndy5 2bndy5 transferred this issue from nRF24/RF24 Nov 9, 2020
@TMRh20 TMRh20 closed this as completed in #5 Mar 12, 2022
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 a pull request may close this issue.

2 participants