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

CircuitPython mode: support for multiple serial ports per board; use serial port to identify #1371

Merged
merged 7 commits into from
Apr 18, 2021

Conversation

dhalbert
Copy link
Contributor

@dhalbert dhalbert commented Mar 17, 2021

Addresses #1352.

CircuitPython can now be built with a second serial port to send data back and forth to the host, in addition to the REPL port. See #1352 for details. This PR identifies the REPL ports correctly.

  • Identify REPL ports by using the adafruit-board-toolkit library (brand new) from pypi.org. The library uses pyserial's serial.tools.list_ports functionality to find ports with particular interface name strings. The REPL ports start with "CircuitPython CDC ". On Linux it uses only the pyserial library. For Windows and MacOS, the scripts for those platforms provided by pyserial need fixing and enhancement, and substitute versions are included in adafruit-board-toolkit. We are PR'ing these fixes back to pyserial.
  • Using the REPL port interface strings means we can identify any CircuitPython board by its REPL interface name. So the code to identify by USB VID/PID can be removed, and does not need to be updated when new boards are added.
  • Dropping USB VID/PID means that all CircuitPython boards are now just labeled as "CircuitPython board". Previously, boards with specific VID/PIDs were named explicitly, and all Adafruit boards were erroneously named "Adafruit CircuitPlayground".
  • The list of CircuitPython modules that should not be filenames has been updated.
  • pyserial>=3.5 must be true for the adafruit-board-toolkit library to work properly. Previously pyserial was pinned at 3.4.
  • adafruit-board-toolkit is now an additional dependency.
  • .gitignore now ignores *~ and TAGS, which are common when using emacs.

Tested on Ubuntu 20.04, Windows 10 Pro 20H2, and MacOS 11.2.2, with two different CircuitPython 6.2.0-beta.3 boards, both with secondary USB CDC channels.

@dhalbert
Copy link
Contributor Author

Just checking, are you planning to continue Python 3.5 support? I can try to roll back adafruit-board-toolkits requirement for 3.6. I see #1127 which proposes to drop 3.5.

@dhalbert
Copy link
Contributor Author

dhalbert commented Mar 17, 2021

I am puzzled by the one failing test above, which is a segmentation fault. If someone with privs on this repo could trigger a re-run, that would be great.

@tjguk
Copy link
Collaborator

tjguk commented Mar 17, 2021

I am puzzled by the one failing test above, which is a segmentation fault. If someone with privs on this repo could trigger a re-run, that would be great.

Done

@ntoll
Copy link
Member

ntoll commented Mar 27, 2021

Hi @dhalbert - I'm sorry I've not been able to get to this sooner, but "real work" has got in the way, so currently blasting through PRs at weekends. A quick heads up, for the duration of beta, we'll be cutting a release every two weeks (with the next release, beta.3 arriving this coming Monday). A bit of new Mu context too: the way we package Mu has changed significantly so we're in a better place for more nimble releases. TL;DR - Mu has a core set of dependencies that MUST be installed for Mu to run. Mu also has a further core set of dependencies that are installed in a virtualenv so that the various modes can run.

This work should definitely be in the beta, but I don't think it'll make Monday's. It would be great to get this into beta.4 in just over a fortnight.

For me there are two things that need resolving, both of which are relatively trivial:

  • PySerial version decision. I'm comfortable with >=3.5, but I'm less sure why PySerial is needed to be "declared" in both the Mu dependencies and virtualenv dependencies. @carlosperate what am I missing here..?
  • The adafruit_board_toolkit module needs to be included in our packaging in some way. I suspect, since it's not a core part of Mu but is definitely a part of CircuitPython mode, that it should be in the virtualenv dependencies. @carlosperate would you agree.

Since the adafruit_board_toolkit isn't referenced as a dependency anywhere, the tests currently fail for me. I suspect the best place to fix this is by adding the module to the mode_packages list in /mu/wheels/__init__.py.

I hope this is all clear. As always, it's such a pleasure to collaborate and I hope you're well given such strange times. 🤗

@dhalbert
Copy link
Contributor Author

  • The adafruit_board_toolkit module needs to be included in our packaging in some way. I suspect, since it's not a core part of Mu but is definitely a part of CircuitPython mode, that it should be in the virtualenv dependencies. @carlosperate would you agree.

Since the adafruit_board_toolkit isn't referenced as a dependency anywhere, the tests currently fail for me. I suspect the best place to fix this is by adding the module to the mode_packages list in /mu/wheels/__init__.py.

Thanks, @ntoll! I've pushed a new commit that updates /mu/wheels/__init__.py to include adafruit-board-toolkit and bumps pyserial to >=3.5. I didn't realize that was another place for dependencies. But does that cover the packaging dependencies? I looked for yet another place where pyserial was a dependency for packaging, and didn't find it.

port_name,
serial_number,
manufacturer,
self.name,
Copy link
Member

Choose a reason for hiding this comment

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

I guess in this case the device will always be identified as "CircuitPython"?
I was hoping to go in the other direction and include more Adafruit board names in this mode to be able to see that in the UI (of course, always falling back to a default name for any device with the right VID, but unknown PID).

Copy link
Contributor Author

@dhalbert dhalbert Mar 29, 2021

Choose a reason for hiding this comment

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

That's right, so it's a bit of a functionality regression. But the current list is woefully incomplete, so most boards were identifying incorrectly as "CircuitPlayground". The board name shows up in the Serial window in the first prompt.

I already thought about adding some board-name lookup capability to the adafruit_board_toolkit. Maybe it would be better to do that as a separate PR in order not to pile too much on this PR? I can work on that soon, I just didn't want to hold up this PR.

Copy link
Member

Choose a reason for hiding this comment

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

the current list is woefully incomplete, so most boards were identifying incorrectly as "CircuitPlayground".

Oh yeah, that's the main thing I wanted to do, populate that list with more board names, and ensure the compatible_board() method was able to first check for existing VID/PID pairs and then fallback if there is a VID/None entry.
(Sounds like a great use case for the new PEP 634 pattern matcher, too bad we need to support 3.5+ :)

I think we can try to match the name here with the PIDs in the original valid_boards list, that way we can keep it and that reduces the diff too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current list is actually very out of date, and all Adafruit boards are already wildcarded to "Adafruit CircuitPlayground". We actually support about 200 boards now.

valid_boards = [
(0x2B04, 0xC00C, None, "Particle Argon"),
(0x2B04, 0xC00D, None, "Particle Boron"),
(0x2B04, 0xC00E, None, "Particle Xenon"),
(0x239A, None, None, "Adafruit CircuitPlayground"),
# Non-Adafruit boards
(0x1209, 0xBAB1, None, "Electronic Cats Meow Meow"),
(0x1209, 0xBAB2, None, "Electronic Cats CatWAN USBStick"),
(0x1209, 0xBAB3, None, "Electronic Cats Bast Pro Mini M0"),
(0x1209, 0xBAB6, None, "Electronic Cats Escornabot Makech"),
(0x1B4F, 0x8D22, None, "SparkFun SAMD21 Mini Breakout"),
(0x1B4F, 0x8D23, None, "SparkFun SAMD21 Dev Breakout"),
(0x1209, 0x2017, None, "Mini SAM M4"),
(0x1209, 0x7102, None, "Mini SAM M0"),
(0x04D8, 0xEC72, None, "XinaBox CC03"),
(0x04D8, 0xEC75, None, "XinaBox CS11"),
(0x04D8, 0xED5E, None, "XinaBox CW03"),
(0x3171, 0x0101, None, "8086.net Commander"),
(0x04D8, 0xED94, None, "PyCubed"),
(0x04D8, 0xEDBE, None, "SAM32"),
(0x1D50, 0x60E8, None, "PewPew Game Console"),
(0x2886, 0x802D, None, "Seeed Wio Terminal"),
(0x2886, 0x002F, None, "Seeed XIAO"),
(0x1B4F, 0x0016, None, "Sparkfun Thing Plus - SAMD51"),
(0x2341, 0x8057, None, "Arduino Nano 33 IoT board"),
(0x04D8, 0xEAD1, None, "DynOSSAT-EDU-EPS"),
(0x04D8, 0xEAD2, None, "DynOSSAT-EDU-OBC"),
(0x1209, 0x4DDD, None, "ODT CP Sapling M0"),
(0x1209, 0x4DDE, None, "ODT CP Sapling M0 w/ SPI Flash"),
(0x239A, 0x80AC, None, "Unexpected Maker FeatherS2"),
(0x303A, 0x8002, None, "Unexpected Maker TinyS2"),
(0x054C, 0x0BC2, None, "Spresense"),
]

Copy link
Contributor Author

@dhalbert dhalbert Mar 29, 2021

Choose a reason for hiding this comment

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

Our answers crossed here. I'm suggesting instead that we remove the board names from the Mu codebase altogether. Instead, you'd call adafruit_board_toolkit.circuitpython_board.name(vid, pid) or something like that and it would give you the name, if it knew the name. Then Mu does not have to be updated constantly as new boards are added.

We will probably do some automation to keep adafruit_board_toolkit up to date in terms of board names. We can get the board names from circuitpython.org.

Copy link
Contributor Author

@dhalbert dhalbert Mar 29, 2021

Choose a reason for hiding this comment

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

remove the board names

and the pid/vids

@carlosperate
Copy link
Member

PySerial version decision. I'm comfortable with >=3.5, but I'm less sure why PySerial is needed to be "declared" in both the Mu dependencies and virtualenv dependencies. @carlosperate what am I missing here..?

Not 100% sure, but I assume it's because "core" files still use pyserial:

from serial import Serial

I think there was a reason not to move that one to QtSerial, but I cannot remember. Maybe @dybber knows, but he will likely be offline for a little while (if you are reading this, no need to worry, just a tag for future reference when you are back).
Hopefully there is something in a git commit or PR, thinking about it, it might be simply because microFs uses pyserial.

The adafruit_board_toolkit module needs to be included in our packaging in some way. I suspect, since it's not a core part of Mu but is definitely a part of CircuitPython mode, that it should be in the virtualenv dependencies. @carlosperate would you agree.

Yeah, a dependency in mu.wheels is the right approach, which I think it has been added in the latest commit, thanks @dhalbert! We should be able to remove it from setup.py now.

@dhalbert
Copy link
Contributor Author

Yeah, a dependency in mu.wheels is the right approach, which I think it has been added in the latest commit, thanks @dhalbert! We should be able to remove it from setup.py now.

Just to be clear, so I should push another commit to remove adafruit-board-toolkit from setup.py?

@carlosperate
Copy link
Member

Yes, although I suspect some test might need to be updated, it's possible we've mocked out the usuage of all the other modules in mu/wheels/__init__.py.

@dhalbert
Copy link
Contributor Author

@carlosperate I tried removing adafruit-board-toolkit from setup.py, and then uninstalled it as well, to see whether a pip3 install . of mu would work. But it doesn't. It fails immediately with ModuleNotFoundError: No module named 'adafruit_board_toolkit'. So I think we need to leave the requirement in setup.py if someone is doing a pip3 install of mu instead of a packaged install.

@dybber
Copy link
Collaborator

dybber commented Mar 30, 2021

The dependency on pySerial comes from these lines:

mu/mu/modes/base.py

Lines 120 to 126 in 7f66a3d

# Using pyserial as a 'hack' to open the port and set DTR
# as QtSerial does not seem to work on some Windows :(
# See issues #281 and #302 for details.
self.serial.close()
pyser = Serial(self.port) # open serial port w/pyserial
pyser.dtr = True
pyser.close()

As far as I know, it's the only place pySerial is used. The mentioned issues predates my involvement, but reading through issue #281, it seems likely that we can actually avoid pySerial as a dependency after all. It seems back then the following was attempted, o set the Data Terminal Ready flag with QSerialPort:

self.ser.dataTerminalReady = True

But I think that is not what you're supposed to do. Instead you have to use the setter:

self.ser.setDataTerminalReady(True)

https://doc.qt.io/qt-5/qserialport.html#dataTerminalReady-prop

@dhalbert
Copy link
Contributor Author

adafruit-board-toolkit does depend on pyserial, specifically its tools.list_ports functionality, so it will need to be included for that reason. If it were just in setup.py, and mu itself did not use pyserial, then it would be brought in by dependency checking. But in the packaging you do, does that also pull in dependencies automatically?

@ntoll
Copy link
Member

ntoll commented Apr 18, 2021

Hi @dhalbert - apologies for the slowness on my part.

I just rebased and "massaged" where the dependencies should be declared. But it all looks good to me and it'll be in the upcoming beta release. All the CI is green so merging.

Thank you so much for your contribution and I hope all is well with you and yours..! 👋

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.

5 participants