add support for multiple pixies to libpixyusb

[tgarc]

This issue was discussed on the wiki here.

I have only done basic testing using the hello_pixy example on my linux box. I tried running two instances in parallel and that works fine. I also tried opening a third one and open() indeed returns a ERROR_BUSY as expected. We should really run some more tests (especially on other platforms) but I haven't looked at what kind of testing infrastructure pixy has atm.

Note that I tried to be careful on handling error conditions. I return the error code unmolested wherever I could and only set the error code manually if no matching pixy device was found. This is different than the pixymon openDevice() implementation which plays it pretty fast and loose :p.

Also, I did a whitespace-cleanup in case you're wondering why some whitespace was removed.

Edit Force pushed an update that makes sure that if libusb_open returns a non-busy error code that the loop exits immediately and the code is returned.

[tgarc]

@richlegrand I'm looking at adding an option to specify UID. But it occurred to me that it wouldn't be straightforward to return the UID since the UID is already a 32-bit unsigned integer whereas the error code is a signed integer. Perhaps USBLink::open could accept an (unsigned) integer pointer that could be unspecified (null by default) or a pointer to a zero integer to open the first available device, and non-zero to grab a specific uid (assuming UIDs are always non-zero). If the passed pointer is non-null, open() will set it to the grabbed UID. That sound okay?

[tgarc]

@richlegrand So there's a bit of a chicken and egg problem here: I can't call 'getUID' until a remote connection to the pixy is initialized via Chirp::setLink and I can't initialize a connection to a specific pixy without querying for it's UID via Chirp::call.

I think I'd have to have some alternative USBLink::open method that returns a list of libusb handles that match the pixy VID/PID and then in PixyInterpreter::init I could iterate over this list, making a connection with each device and querying for it's uid there. Let me know if you have any thoughts.

[richlegrand]

How about something like this--

int32_t open(uint32_t uid=0); // 0 means first available uint32_t getUID(); // returns UID of device we're connected to, 0 upon error

On Sat, Jun 3, 2017 at 1:46 PM, tomas garcia notifications@github.com wrote:

@richlegrand https://github.com/richlegrand I'm looking at adding an option to specify UID. But it occurred to me that it wouldn't be straightforward to return the UID since the UID is already a 32-bit unsigned integer whereas the error code is a signed integer. Any thoughts on this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/charmedlabs/pixy/pull/33#issuecomment-305993995, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3vDTIv3JieoUtlFlqpuFQOaRJGKhdkks5sAan1gaJpZM4NpobT .

-- Charmed Labs www.charmedlabs.com

[richlegrand]

You definitely will need to modify USBLink open -- that's what I was suggesting with

int32_t open(uint32_t uid=0); // 0 means first available

Once you're connected you can call getUID.

I'm not seeing the chicken-egg.....

On Sat, Jun 3, 2017 at 3:32 PM, Rich LeGrand rich@charmedlabs.com wrote:

How about something like this--

int32_t open(uint32_t uid=0); // 0 means first available uint32_t getUID(); // returns UID of device we're connected to, 0 upon error

On Sat, Jun 3, 2017 at 1:46 PM, tomas garcia notifications@github.com wrote:

@richlegrand https://github.com/richlegrand I'm looking at adding an option to specify UID. But it occurred to me that it wouldn't be straightforward to return the UID since the UID is already a 32-bit unsigned integer whereas the error code is a signed integer. Any thoughts on this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/charmedlabs/pixy/pull/33#issuecomment-305993995, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3vDTIv3JieoUtlFlqpuFQOaRJGKhdkks5sAan1gaJpZM4NpobT .

-- Charmed Labs www.charmedlabs.com

-- Charmed Labs www.charmedlabs.com

[richlegrand]

Sorry... feel free to give me a call.

I was working from memory.

pixy_init will need to take an optional uid argument too.

On Sat, Jun 3, 2017 at 3:53 PM, Rich LeGrand rich@charmedlabs.com wrote:

You definitely will need to modify USBLink open -- that's what I was suggesting with

int32_t open(uint32_t uid=0); // 0 means first available

Once you're connected you can call getUID.

I'm not seeing the chicken-egg.....

On Sat, Jun 3, 2017 at 3:32 PM, Rich LeGrand rich@charmedlabs.com wrote:

How about something like this--

int32_t open(uint32_t uid=0); // 0 means first available uint32_t getUID(); // returns UID of device we're connected to, 0 upon error

On Sat, Jun 3, 2017 at 1:46 PM, tomas garcia notifications@github.com wrote:

@richlegrand https://github.com/richlegrand I'm looking at adding an option to specify UID. But it occurred to me that it wouldn't be straightforward to return the UID since the UID is already a 32-bit unsigned integer whereas the error code is a signed integer. Any thoughts on this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/charmedlabs/pixy/pull/33#issuecomment-305993995, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3vDTIv3JieoUtlFlqpuFQOaRJGKhdkks5sAan1gaJpZM4NpobT .

-- Charmed Labs www.charmedlabs.com

-- Charmed Labs www.charmedlabs.com

-- Charmed Labs www.charmedlabs.com

[tgarc]

int32_t open(uint32_t uid=0); // 0 means first available Once you're connected you can call getUID. I'm not seeing the chicken-egg.....

The chicken-egg is that you can't call getUID within USBLink::open - a link needs to be established with Chirp::setLink first. So the uid code will need to be implemented in PixyInterpreter::init instead. Anyway, I think I've figured something out.

The getUID function is interesting, it seems useful to expose the uid this way.

[richlegrand]

Hey Tomas, Thanks for this. It would be nice to keep the USB stuff confined to USBLink. How about this---

We change USBLink::open so that it takes an optional int:

int open(uint ord=0);

the int tells open to open the 1st enumerated pixy (ord==0), 2nd (ord==1), etc. If there is no such ordinal pixy in the enumeration, it returns error -- PIXY_ERROR_NO_DEVICE

We can then do something similar to what you did in pixyinterpreter:

For (i=0; i<MAXPIXYS; i++) { res = link.open(i); if (res==PIXY_ERROR_NODEVICE ) break; else { receiver new chirpReceiver(&link_, &interpreter);

// call getUID

send_command(...)

// If uid's are unequal, clean up receiver, continue

// If they’re equal, break…

} }

(don't take me too literally here... just some sketchy code to describe what I"m thinking)

BTW did you verify the getUID returns a unique ID (at least when used on the 2 pixys you have)?

thanks! --rich

[tgarc]

It would be nice to keep the USB stuff confined to USBLink.

Agreed. That's why I feel like my solution is hacky.

BTW did you verify the getUID returns a unique ID (at least when used on the 2 pixys you have)?

Yeap, works just fine.

How about this--- We change USBLink::open so that it takes an optional int: ... the int tells open to open the 1st enumerated pix

That could work, the only potential issue I see is that the list returned by libusb_get_device_list may not always return devices in the same order (especially across all platforms). I could ask on the forums though (a quick search didn't pull up anything).

Another potential solution would be to drop m_context from the USBLink object. I can't see any good reason why it's been included and it only complicates the code. We can always add it back later. If it was removed I could just use USBLinks instead of libusb_device_handlers and wouldn't have to touch libusb stuff within the pixyinterpreter code. The only ugliness is that pixyinterpreter would still be calling open() multiple times but...consider it an undocumented feature :p.

EDIT After a further look at the codebase it looks like m_context is pretty ubiquitous, so I don't want to remove it from usblink if for nothing else than to avoid breaking consistency. I will try your suggested approach

EDIT FWIW, your suggestion does work on linux.

[richlegrand]

Actually, the order of enumeration doesn't affect much. It only affects (possibly) how fast we find the device we're looking for. At any rate, it isn't a concern.... the solution I described as far as I can tell should work. :)

there may be a way to enumerate differently (removing m_context), but I see no good reason to rewrite. Anyway, let me know your thoughts.

On Thu, Jun 8, 2017 at 10:11 PM, tomas garcia notifications@github.com wrote:

It would be nice to keep the USB stuff confined to USBLink.

Agreed. That's why I feel like my solution is hacky.

BTW did you verify the getUID returns a unique ID (at least when used on the 2 pixys you have)?

Yeap, works just fine.

How about this--- We change USBLink::open so that it takes an optional int: ... the int tells open to open the 1st enumerated pix

That could work, the only potential issue I see is that the list returned by libusb_get_device_list may not always return devices in the same order (especially across all platforms). I could ask on the forums though (a quick search didn't pull up anything).

Another potential solution would be to drop m_context from the USBLink object. I can't see any good reason why it's been included and it only complicates the code. We can always add it back later. If it was removed I could just use USBLink's instead of libusb_device_handlers and wouldn't have to touch libusb stuff within the pixyinterpreter code. The only ugliness is that pixyinterpreter would still be calling open() multiple times but...consider it an undocumented feature :p.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/charmedlabs/pixy/pull/33#issuecomment-307283920, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3vDYsUKz8HzrY4Nu7KpGoAYpd_hHZEks5sCLfsgaJpZM4NpobT .

-- Charmed Labs www.charmedlabs.com

[richlegrand]

Ok, you're worried that it will return different orderings from call to call... hmmm. I hadn't considered that.... it sees pretty unlikely though.

On Fri, Jun 9, 2017 at 8:14 AM, Rich LeGrand rich@charmedlabs.com wrote:

Actually, the order of enumeration doesn't affect much. It only affects (possibly) how fast we find the device we're looking for. At any rate, it isn't a concern.... the solution I described as far as I can tell should work. :)

there may be a way to enumerate differently (removing m_context), but I see no good reason to rewrite. Anyway, let me know your thoughts.

On Thu, Jun 8, 2017 at 10:11 PM, tomas garcia notifications@github.com wrote:

It would be nice to keep the USB stuff confined to USBLink.

Agreed. That's why I feel like my solution is hacky.

BTW did you verify the getUID returns a unique ID (at least when used on the 2 pixys you have)?

Yeap, works just fine.

How about this--- We change USBLink::open so that it takes an optional int: ... the int tells open to open the 1st enumerated pix

That could work, the only potential issue I see is that the list returned by libusb_get_device_list may not always return devices in the same order (especially across all platforms). I could ask on the forums though (a quick search didn't pull up anything).

Another potential solution would be to drop m_context from the USBLink object. I can't see any good reason why it's been included and it only complicates the code. We can always add it back later. If it was removed I could just use USBLink's instead of libusb_device_handlers and wouldn't have to touch libusb stuff within the pixyinterpreter code. The only ugliness is that pixyinterpreter would still be calling open() multiple times but...consider it an undocumented feature :p.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/charmedlabs/pixy/pull/33#issuecomment-307283920, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3vDYsUKz8HzrY4Nu7KpGoAYpd_hHZEks5sCLfsgaJpZM4NpobT .

-- Charmed Labs www.charmedlabs.com

-- Charmed Labs www.charmedlabs.com

[richlegrand]

I see what you're talking about now with m_context .... took me awhile! yeah, seems like a good approach. Give it a try!

edit: oh, I see that edits to your message now....

On Fri, Jun 9, 2017 at 8:24 AM, Rich LeGrand rich@charmedlabs.com wrote:

Ok, you're worried that it will return different orderings from call to call... hmmm. I hadn't considered that.... it sees pretty unlikely though.

On Fri, Jun 9, 2017 at 8:14 AM, Rich LeGrand rich@charmedlabs.com wrote:

Actually, the order of enumeration doesn't affect much. It only affects (possibly) how fast we find the device we're looking for. At any rate, it isn't a concern.... the solution I described as far as I can tell should work. :)

there may be a way to enumerate differently (removing m_context), but I see no good reason to rewrite. Anyway, let me know your thoughts.

On Thu, Jun 8, 2017 at 10:11 PM, tomas garcia notifications@github.com wrote:

It would be nice to keep the USB stuff confined to USBLink.

Agreed. That's why I feel like my solution is hacky.

BTW did you verify the getUID returns a unique ID (at least when used on the 2 pixys you have)?

Yeap, works just fine.

How about this--- We change USBLink::open so that it takes an optional int: ... the int tells open to open the 1st enumerated pix

That could work, the only potential issue I see is that the list returned by libusb_get_device_list may not always return devices in the same order (especially across all platforms). I could ask on the forums though (a quick search didn't pull up anything).

Another potential solution would be to drop m_context from the USBLink object. I can't see any good reason why it's been included and it only complicates the code. We can always add it back later. If it was removed I could just use USBLink's instead of libusb_device_handlers and wouldn't have to touch libusb stuff within the pixyinterpreter code. The only ugliness is that pixyinterpreter would still be calling open() multiple times but...consider it an undocumented feature :p.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/charmedlabs/pixy/pull/33#issuecomment-307283920, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3vDYsUKz8HzrY4Nu7KpGoAYpd_hHZEks5sCLfsgaJpZM4NpobT .

-- Charmed Labs www.charmedlabs.com

-- Charmed Labs www.charmedlabs.com

-- Charmed Labs www.charmedlabs.com

[richlegrand]

You could do something where you open the device and hold onto it temporarily (if that makes sense). It get's around the ordering concern and doesn't require modification of USBLink...

On Fri, Jun 9, 2017 at 8:31 AM, Rich LeGrand rich@charmedlabs.com wrote:

I see what you're talking about now with m_context .... took me awhile! yeah, seems like a good approach. Give it a try!

On Fri, Jun 9, 2017 at 8:24 AM, Rich LeGrand rich@charmedlabs.com wrote:

Ok, you're worried that it will return different orderings from call to call... hmmm. I hadn't considered that.... it sees pretty unlikely though.

On Fri, Jun 9, 2017 at 8:14 AM, Rich LeGrand rich@charmedlabs.com wrote:

Actually, the order of enumeration doesn't affect much. It only affects (possibly) how fast we find the device we're looking for. At any rate, it isn't a concern.... the solution I described as far as I can tell should work. :)

there may be a way to enumerate differently (removing m_context), but I see no good reason to rewrite. Anyway, let me know your thoughts.

On Thu, Jun 8, 2017 at 10:11 PM, tomas garcia notifications@github.com wrote:

It would be nice to keep the USB stuff confined to USBLink.

Agreed. That's why I feel like my solution is hacky.

BTW did you verify the getUID returns a unique ID (at least when used on the 2 pixys you have)?

Yeap, works just fine.

How about this--- We change USBLink::open so that it takes an optional int: ... the int tells open to open the 1st enumerated pix

That could work, the only potential issue I see is that the list returned by libusb_get_device_list may not always return devices in the same order (especially across all platforms). I could ask on the forums though (a quick search didn't pull up anything).

Another potential solution would be to drop m_context from the USBLink object. I can't see any good reason why it's been included and it only complicates the code. We can always add it back later. If it was removed I could just use USBLink's instead of libusb_device_handlers and wouldn't have to touch libusb stuff within the pixyinterpreter code. The only ugliness is that pixyinterpreter would still be calling open() multiple times but...consider it an undocumented feature :p.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/charmedlabs/pixy/pull/33#issuecomment-307283920, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3vDYsUKz8HzrY4Nu7KpGoAYpd_hHZEks5sCLfsgaJpZM4NpobT .

-- Charmed Labs www.charmedlabs.com

-- Charmed Labs www.charmedlabs.com

-- Charmed Labs www.charmedlabs.com

-- Charmed Labs www.charmedlabs.com

[tgarc]

@richlegrand I implemented your approach (I think it has the lowest impact to the rest of the code). Do you have a mac by chance? Either way, try it out.

For a couple subtle reasons (including the 'this may not work on all platforms' caveat) it's not ideal, but the other approaches I thought of would require either that m_context be removed (in which case libusb automatically manages opening and closing the 'default' context), or that some code be added so that multiple USBLinks could be associated with the same m_context (something like a reference count management of m_context which is exactly what libusb does for the default context).

Either way, I think the initial commit of this PR is A-OK, doesn't introduce any additional issues, and allows for use of multiple pixies - seems ready for merging. If we want to give the getuid stuff a little bit more thought, I can just move that to a separate PR.

[richlegrand]

Hey Tomas, Looks good--- thanks! Note, you should put optional args in the declaration not the definition. I'm not sure what putting it only in the definition does. In general, I think putting the optional arg in the declaration (not definition) is the preferred way -- the compiler will look at the declaration and see no optional arg and send an error if you leave the arg off. If found this: https://stackoverflow.com/questions/4989483/where-to-put-default-parameter-value-in-c

For the C declaration, we can't use the default arg, but we can for the C++ declaration, which will require some ifdef __cplusplus logic around pixy_init decaration in pixy.h

Also, please describe the usage of uid in the doxygen tags for pixy_init. (0==first available)

I sort of quickly proposed a different approach where we keep USBLink as-is, and in Interpreter we open a Pixy device, check it's uid and if it doesn't match, we keep it open. We than call USBLink::open again, which should get us the next device, etc. At the end we close all devices we're holding onto (except the one that matches our uid.) Anyway, you mentioned the ordering concern, and I'm not sure if it's something we should worry about. If you're into implementing this "new" method, go for it. If not, that's fine too....

[tgarc]

Note, you should put optional args in the declaration not the definition.

Thanks, it's been a while since I've written c++...

Also, please describe the usage of uid in the doxygen tags

Sure thing

I sort of quickly proposed a different approach where we keep USBLink as-is, ... We than call USBLink::open again which should get us the next device

That's kinda the approach I was going for initially (except I was holding onto the libusb_device_handle/m_handle). But if I hold onto multiple USBLinks, each one creates their own libusb_context which may have side effects I don't understand (and may not be worth digging into right now). I don't mean to be nitpicky, but I'm new at this code and libusb so I'd like to keep my changes minimal and avoid using things I don't completely understand.

[richlegrand]

Just throwing it out there for discussion --- agreed there may be gotchas with this approach too.

thanks!

On Tue, Jun 13, 2017 at 9:49 AM, tomas garcia notifications@github.com wrote:

Note, you should put optional args in the declaration not the definition.

Thanks, it's been a while since I've written c++...

Also, please describe the usage of uid in the doxygen tags

Sure thing

I sort of quickly proposed a different approach where we keep USBLink as-is, ... We than call USBLink::open again which should get us the next device

That's kinda the approach I was going for initially (except I was holding onto the libusb_device_handle/m_handle). But if I hold onto multiple USBLinks, each one creates their own libusb_context which may have side effects I don't understand (and may not be worth digging into right now). I don't mean to be nitpicky, but I'm new at this code and libusb so I'm hesitant to make changes I don't completely understand.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/charmedlabs/pixy/pull/33#issuecomment-308141282, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3vDY2RcqsOfAgscO4TYegrkX94sEUkks5sDqFfgaJpZM4NpobT .

-- Charmed Labs www.charmedlabs.com

[tgarc]

@richlegrand quick question. You mentioned this in a previous comment:

For the C declaration, we can't use the default arg, but we can for the C++ declaration, which will require some ifdef __cplusplus logic around pixy_init decaration in pixy.h

Could you explain this a bit more? There's already an #ifdef __cplusplus around the entirety of the pixy.h declarations, do I need to do something further?

[richlegrand]

Right -- just disregard. thanks

On Sun, Jun 25, 2017 at 2:27 PM, tomas garcia notifications@github.com wrote:

@richlegrand https://github.com/richlegrand quick question. You mentioned this in a previous comment:

For the C declaration, we can't use the default arg, but we can for the C++ declaration, which will require some ifdef __cplusplus logic around pixy_init decaration in pixy.h

Could you explain this a bit more? There's already an #ifdef __cplusplus around the entirety of the pixy.h, do I need to do something further?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/charmedlabs/pixy/pull/33#issuecomment-310922729, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3vDUhQivytlnFuJtgazWyUZJRnkgKAks5sHrS2gaJpZM4NpobT .

-- Charmed Labs www.charmedlabs.com

[tgarc]

@richlegrand I've created a develop branch and am merging this PR to that branch. (I saw a development branch already present but it's really outdated and I didn't want to just assume I could update it.) Let me know if you find any issues.