BrianPugh / belay

Belay is a python library that enables the rapid development of projects that interact with hardware via a micropython-compatible board.
Apache License 2.0
240 stars 13 forks source link

UsbSpecifier for fine-grained device target selection. #122

Closed BrianPugh closed 1 year ago

BrianPugh commented 1 year ago

Continuing discussion from #60 . There is a desire to be able to specify the micropython board by attributes like pid/vid/serial_number. This PR aims to address that.

This PR introduces the class UsbSpecifier that takes in a variety of optional keyword arguments. The resulting object can be passed into belay.Device. If an attached board matches all of the specified keyword arguments (like serial_number, pid, etc), then it will be used. If no device is found, or if multiple devices match the provided specifier, then an exception is raised.

@raveslave, this PR is not yet complete, but so far it's in a working state. Would like feedback on naming/usage, and if it satisfies your needs. I'm not a huge fan of the current name UsbSpecifier and am open to suggestions. Aside from feedback, items that need to be completed before merging this PR:

  1. Update docs
  2. Maybe add new CLI features for making this easier; maybe modify the existing belay identify command.

Example usage:

from belay import Device, UsbSpecifier
device = Device(
    UsbSpecifier(
        serial_number="abc123",
    )
)
codecov-commenter commented 1 year ago

Codecov Report

Merging #122 (c6463eb) into main (a8b5912) will increase coverage by 0.35%. The diff coverage is 88.63%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #122      +/-   ##
==========================================
+ Coverage   76.63%   76.98%   +0.35%     
==========================================
  Files          32       33       +1     
  Lines        1318     1360      +42     
  Branches      213      222       +9     
==========================================
+ Hits         1010     1047      +37     
- Misses        240      244       +4     
- Partials       68       69       +1     
Flag Coverage Δ
unittests 76.91% <88.63%> (+0.35%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
belay/helpers.py 90.00% <66.66%> (-4.12%) :arrow_down:
belay/usb_specifier.py 88.57% <88.57%> (ø)
belay/__init__.py 100.00% <100.00%> (ø)
belay/exceptions.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

BrianPugh commented 1 year ago

@raveslave any thoughts?

Also additional notes for related features: I recently just learned that you can have .gitignore in subdirectories. So I would be good with having:

.belay/
    user/
        .gitignore  # contents: "*"
        files-that-shoud-not-be-committed
    dependencies/
        stuff-that-already-exists-and-should-be-comitted

In doing so, we can store user device-configuration data in .belay/user/

raveslave commented 1 year ago

checking now, sorry for the delay. looks really promising!

raveslave commented 1 year ago

re:

In doing so, we can store user device-configuration data in .belay/user/

yes, you rarely use ignores in subfolders, but a good way to keep status clean for "local cache", I prefer it in repo rather than ~/ as in many other projects. so if you feel like adding this, I think its a great idea. ps. we should just make sure it's clear when connecting (log? and print in cli) that it loads from that file, i.e. like ssh -v to see if your keys are being used or not... just so you dont get lost why its connecting to something automagically

BrianPugh commented 1 year ago

Thanks for all the good feedback!

maybe multiple device matches should have its own error type (instead of DeviceNotFoundError)

✅ I added a new exception InsufficientSpecifierError if multiple devices match the provided UsbSpecifier. I'm also fine to change the name if you can think of a better one.

UsbSpecifier should offer to pass-in bus and address to allow you to pick a specific hub and/or port in a usb-hub

I'm trying to not introduce an unnecessary dependency; is location from pyserial equivalent? Example output (copied from your previous post, but just including here for completeness):

{'description': 'Board in FS mode',
 'device': '/dev/cu.usbmodem147201',
 'hwid': 'USB VID:PID=1B4F:0026 SER=e46030f803311824 LOCATION=20-7.2',
 'interface': None,
 'location': '20-7.2',
 'manufacturer': 'MicroPython',
 'name': 'cu.usbmodem147201',
 'pid': 38,
 'product': 'Board in FS mode',
 'serial_number': 'e46030f803311824',
 'vid': 6991}

If it is the same, would we want to keep the name location? If it's not, we can add a dependency like pyusb.

  • Same as above, those properties should be populated when searching by any other parameter so you can conveniently access bus/address (and include it if printed out)
  • I like the idea of the CLI having a convenient list/identify feature, if this list is interactive, selecting one could offer to set a local environment var to keep it sticky throught the session?

So lets talk about the interactive CLI bit. Here's what I'm thinking.

  1. There's the existing belay identify command that basically blinks a specified LED (assuming your board has an LED). This makes it easy to identify which board you're talking to. I'm just mentioning this here because it should probably be rolled into this feature.
  2. For the sake of argument, lets call this new command belay select. We can use questionary to do the interactive heavy lifting. Maybe after selecting, promt the user if they want to blink an led and then either confirm or go back to the menu?
  3. Once a device is selected, we have 2 options of saving this info: Save it to a file in .belay/user, or save it to an env var. The issue with saving it to .belay/user is that the script might be "just a script" and not really in a good project structure. So for this, I believe the env var is the way to go. Unfortunately, a child process can't update it's parents environment, so this might be a bit of a dead end. An alternative is for belay select to print out for the user to run export BELAY_DEVICE=some_serialization_of_specified_device.
  4. Assuming (3) gets solved, we could make the port/device-specifier argument to Device as optional, and have it fall back to the env var.

ps. we should just make sure it's clear when connecting (log? and print in cli) that it loads from that file, i.e. like ssh -v to see if your keys are being used or not... just so you dont get lost why its connecting to something automagically

I think the best long-term solution is to add a proper logger, and then allow a verbosity level to be set. I think this would be useful to belay-users and belay-library-developers alike. For the sake of scope, we'll save this for another PR.

I think the action plan is going to look like this:

  1. Lets iron out any UsbSpecifier details in this PR. Things like the bus/address stuff you mentioned, and then also ways of serializing/deserializing the data in prep for step (2).
  2. The interactive cli belay select will be next PR. This PR will also contain the ability to specify a belay.Device() without any arguments (falling back to env var or whatever).
  3. A proper logger will come after that in another PR.
raveslave commented 1 year ago

great re: InsufficientSpecifierError, I think the name works!

actually never used 'location' in pyserial but seems to represent the bus and address topology seen in pyusb, but in its own way. so location will be perfect to pin two identical devices connected at the same time. not sure why my print became 20-7.2 when the format seems to be <bus>-<port>[-<port>], but the docs at least claim its unique so we're good!

raveslave commented 1 year ago

regarding the CLI

  1. belay identify is great and I think it should include the led blink

  2. belay select sounds good. no real opinion about prompt libs vs native/homebrew. since the user will be in the cli at this point, I like the idea of the extra blink step as a confirmation step.

  3. true regarding the scope/current working dir.. maybe best to just drop this idea and let the user manage this via source .env or similar

  4. for a while I was thinking pyserial's hwid would be the unique string we want (in setups where bus/hub & port are part of the unique device identifier). but it's simple to just explicitly pass-in all args. drawback is that it gets less obvious what to store in your env variable, or did you have any ideas here?

great stuff! plan 1-2 check, and agree 3 would be its own PR

BrianPugh commented 1 year ago

All sounds good; for this PR I'll do the remaining:

  1. Add the location field.
  2. Add UsbSpecifier serialization/deserialization in preparation for the other features.
BrianPugh commented 1 year ago

so I've been a bit slow on this (I just had my first baby 🥳), but I added location field to UsbSpecifier.

raveslave commented 1 year ago

wow! congrats to you+fam+kid!!

re serialization, I think the easiest approach now is to offer a way to retrieve a specifier that can be easily copied to code and/or an env variable. to me the nicest feature here is to completely remove the needd to ever do ls /dev/tty [tab tab] again, just run the CLI, see whats there, then pass-in the attributes needed in the code and you're done.

documentation-wise, I think its either a matter of locking down on vid+pid and possibly serial. and in the more rare case of you having two or more with an ambiguous serial, you make use of the usb-bus port/hub configuration (which can sometimes be nice to read-back if you require a certain device to be connected in a specific port on a powered hub)

about default Device(), this one is of course tricky, if there is only one device I think it should return it, but how does it know. I know that 'mpremote list' tries to do this, but on my mac it shows the build-in bluetooth modem as well.. without doing some fuzzy micropy detection logic, I think its too risky to simply return the first CDC-ACM device on the bus (unless there is a solid way of detecting the repl)

sounds like a good idea to replace list_devices as its now part of the more powerful/flexible approach

BrianPugh commented 1 year ago

cool! These will be my action items/thoughts:

  1. THIS PR: Add serialization/deserialization of UsbSpecifier to a dict. I think pydantic offers this, so this is super easy. The intent will be that this dictionary-string representation could be stored in an env var like BELAY_DEVICE='{"vid": "0123", "pid": "4567"}'.
  2. THIS PR: replace the body of list_devices with list_usb_specifiers. @roaldarbol let me know if you have any objections.
  3. NEXT PR: In belay.Device, if no device is specified, attempt to access env var BELAY_DEVICE and see if it decodes to a meaningful UsbSpecifier. If not, error our. Later we could add an additional fallback to try and find an available device with fuzzy logic, but it'd have the shortcomings you mentioned.
  4. NEXT PR: I'm in favor of belay select outputing both easy-to-copy-and-paste python code, as well as env var export snippet.
roaldarbol commented 1 year ago

Sorry, have been away on holiday. :-) All sounds good to me! Sounds like a great upgrade - good work guys!

BrianPugh commented 1 year ago

Alright, so notes for future PRs:

raveslave commented 1 year ago

awesome work brian!