AllenNeuralDynamics / python-newscale

Python library for controlling the micromanipulator systems from New Scale Technologies
MIT License
0 stars 0 forks source link

serial abstraction layer (sal) #7

Open chronopoulos opened 1 year ago

chronopoulos commented 1 year ago

this PR introduces a "serial abstraction layer" called NewScaleSerial, which represents a platform-agnostic abstraction of the serial pipe to the USBInterface (M3-USB-3.1-EP). the NewScaleSerial class exposes a class method get_instances() which returns the instances of a serial port which match the VID and PID of the newscale device; on linux this is a pyserial "Serial" object, on windows it's a USBXpressDevice. instances of NewScaleSerial expose methods like get_port_name, get_serial_number, set_baudrate, set_timeout, write, and readLine which call the appropriate subroutines depending on the platform. a NewScaleSerial instance can be used to instantiate a USBInterface object, and then the rest of the library works in the same way.

examples/ have been updated, and i added back_and_forth_3d.py as a simple, repeatable (no random numbers) alternative to multistage_test.py. all examples have now been tested on both windows and linux.

Poofjunior commented 1 year ago

License Stuff

Does usbxpress.py have a license associated with it? I can't find a good source for where this file came from apart from the answer to this forum question. If there is no license, then we should probably reference their "Master Software License Agreement."

Since the code is present as source code, it looks like this file might qualify as open source code, except that the file itself is not tagged as such. See:

“Silicon Labs Open Source Code” means Software created by Silicon Labs and which is (a) delivered to Licensee in Source Code format, (b) is identified as open source code and (c) states that use of the software is subject to the terms of this Agreement.

In our case, I don't know about (b) and (c).

Is there a general license for code that appears in the forums?

chronopoulos commented 1 year ago

appreciate the review!

  1. licenses on usbxpress.py

great point, i think this may actually fall under section 4.2.5 of the MSLA. i have added a copyright header and license reference as of 0dd801d, let me know what you think.

  1. adding native usb functionality, rather than replacing the existing COM port functionality.

ok, while i don't think we should spend too much effort on supporting cases that New Scale doesn't support (like aftermarket PID modification), this is simple enough to keep that i agree with you. check out c4d61d9; i did something a little different than what you suggest but it achieves the same result: i just extended the NewScaleSerial.get_instances to include checks for the modified PID (0xea60), and then the rest is identical to the pyserial library as we use it in linux.

  1. hiding the get_instances calls that the user doesn't need to do if they know the serial number.

i think i disagree with this suggestion, because asking "what new scale devices are connected right now" is a very common use case (i'm calling this in parallax to populate a drop-down menu with the serial numbers of available stages).

Poofjunior commented 1 year ago

I can't speak to adding a license to code where there is no license in it already, but this might be the best we can do for now.

check out https://github.com/AllenNeuralDynamics/python-newscale/commit/c4d61d93d2d3b282e676d799e7a4cd30cf6f2c5d

This looks great. That way we don't break linux functionality that we got for free.

i think i disagree with this suggestion, because asking "what new scale devices are connected right now" is a very common use case (i'm calling this in parallax to populate a drop-down menu with the serial numbers of available stages).

Whoop I meant the following.

Let's keep what you have, i.e:

devices = NewScaleSerial.get_instances()

But when it comes to instantiating a device, let's support both of these use cases:

my_xyz_stage = M3USBInterface(devices[0]) 
# OR
my_xyz_stage = M3USBInterfaces(serial_num=46109)

That way you can support:

  1. drop-down menu to list out devices (good for interactive prototyping)
  2. a way of instantiating the device if you already know the serial number (good for permanent setups with a fixed configuration)

To do the latter, we just push the NewScaleSerial instantiation into the class USBInterface's __init__, rather than pass in an existing one as a parameter.

The only other thing I might suggest is considering making get_instances a standalone function to this module. Here's a similar example from another package. Up to you on this one though!

chronopoulos commented 1 year ago

ok, i pushed a couple of changes: 0dd801d adds copyright and license, and c4d61d9 adds support for the provisioned devices.

my_xyz_stage = M3USBInterface(devices[0])

are you aware that M3USBInterface is not used anywhere else in the library? it seems to only add setting/getting of baud rate, which i am currently handling at the NewscaleSerial level. i feel like we should either (a) remove these methods/class entirely or (b) move them into USBSerialInterface as a passthrough to NewscaleSerial. what do you think?

finally, i took a stab at implementing instantiation given a known serial number, but wasn't able to come up with anything i was happy with. maybe you could try and propose something?