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

Closes USB comms properly to allow communication while booting. #386

Closed
wants to merge 3 commits into from

Conversation

tcr
Copy link
Member

@tcr tcr commented Oct 19, 2015

See #346, #291.

Only tested with t2 list for now.

@johnnyman727
Copy link
Contributor

@tcr thanks for working on this!

So we have some infrastructure in place to make sure all Tessel connections get closed. Essentially, every command run will end with controller.closeTesselConnections which ultimately calls tessel.close and that function, if it has a USB connection, will ultimately call USBConnection._close.

It seems as though the significant part of your PR is calling self.epIn.stopPoll before releasing the interface. Can we just include that portion of your PR to have the same effect? It seems like the rest of the PR is just making sure connections get closed which they already do.

Additionally, I believe this will throw an error if you have any pending packets to be sent over USB, won't it?

@tcr
Copy link
Member Author

tcr commented Oct 20, 2015

@johnnyman727 controller.closeTesselConnections will not be called with Tessels that don't identify themselves, i.e. a Tessel while booting does not wind up in the list of Tessels that are closed. How do we deal with that?

As for your second question, look at lib/usb_connection.js line 181 in my change. Essentially, given all transfers will timeout, it simply spin-waits until the last transfer is complete to resolve it. This is hacky, but reliable :)

@rwaldron
Copy link
Contributor

@tcr need to run grunt tests and tooling locally

@@ -97,11 +97,13 @@ Tessel.prototype.simpleExec = function(command) {
var self = this;
return new Promise(function(resolve, reject) {
// Stop processes and delete everything in the folder
return self.connection.exec(command)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you need to remove this return?

@johnnyman727
Copy link
Contributor

@tcr it works great on my machine! It reported when I had Tessels booting and they continue to work once they were up.

I added a few comments to clean up the code a bit before merging.

Thanks!

@johnnyman727 johnnyman727 mentioned this pull request Oct 23, 2015
35 tasks
@Frijol
Copy link
Member

Frijol commented Oct 23, 2015

Got this while working from your branch, is it from the branch itself or something else?

~  t2 wifi
INFO Looking for your Tessel...
WARN 1 Tessel device connected by USB may still be booting.
WARN No Authorized Tessels Found.
events.js:141
      throw er; // Unhandled 'error' event
            ^
Error: Connection was already closed...
    at USB.Connection._write (/Users/frijol/t2-cli/lib/usb_connection.js:70:14)
    at doWrite (_stream_writable.js:291:12)
    at clearBuffer (_stream_writable.js:401:7)
    at onwrite (_stream_writable.js:336:7)
    at OutEndpoint.WritableState.onwrite (_stream_writable.js:88:5)
    at Transfer.callback (/Users/frijol/t2-cli/node_modules/usb/usb.js:343:14)

@Frijol
Copy link
Member

Frijol commented Oct 23, 2015

Also, getting false positives from this branch where it says "may still be booting" but really it's booted but I'm having USB issues. Not sure if we want to address this or assume USB should be working properly

@johnnyman727
Copy link
Contributor

I also just had the case where it had definitely finished booting but then the CLI reported that it was still booting. This PR will need some thorough tests.

@johnnyman727
Copy link
Contributor

Also related tessel/t2-firmware#119

@johnnyman727
Copy link
Contributor

@tcr I've superceded this PR with #400. It takes some of the logic from this PR, some of the logic from #291 and some new logic to achieve the correct result. It also needs tessel/t2-firmware#118 merged.

@Frijol Frijol deleted the tcr-usb branch October 26, 2015 15:19
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