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

Process USB vendor requests in system thread #1323

Merged

Conversation

@avtolstoy
Copy link
Member

commented May 8, 2017

Problem

Most of the currently supported vendor requests should be executed on system thread instead of being processed in ISR.

Solution

Utilize SystemISRTaskQueue for vendor requests.

Steps to Test

N/A

Example App

N/A

References


Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA (Info here)
  • Problem and Solution clearly stated
  • Run unit/integration/application tests on device
  • Added documentation
  • Added to CHANGELOG.md after merging (add links to docs and issues)

Internal

  • [PR #1323] USB vendor requests should be executed on system thread instead of being processed in ISR.

@avtolstoy avtolstoy requested a review from sergeuz May 8, 2017

@avtolstoy avtolstoy added this to the 0.7.0 milestone May 8, 2017

@@ -181,10 +170,13 @@ uint8_t SystemControlInterface::handleVendorRequest(HAL_USB_SetupRequest* req) {
} else {
// Return as string
String id = System.deviceID();

This comment has been minimized.

Copy link
@sergeuz

sergeuz May 8, 2017

Member

Unused variable

@@ -66,6 +67,7 @@ typedef enum USBRequestResult {
typedef struct USBRequest {
size_t size; // Structure size
int type; // Request type (as defined by USBRequestType enum)
uint16_t value; // wValue field

This comment has been minimized.

Copy link
@sergeuz

sergeuz May 8, 2017

Member

This change breaks ABI (new fields should be added at the end of the structure)

@@ -279,6 +279,7 @@ class ManagedNetworkInterface : public NetworkInterface
console.loop();
}
#if PLATFORM_THREADING
SystemISRTaskQueue.process();

This comment has been minimized.

Copy link
@sergeuz
}

case USB_REQUEST_LISTENING_MODE: {
setRequestResult(req, result);

This comment has been minimized.

Copy link
@sergeuz

sergeuz May 8, 2017

Member

Is there any problem with setting a result after calling WiFi.listen()? I just would like to see error handling unified in this function (if possible), i.e. make all case blocks to either call setRequestResult() and return, or assign to result variable.

default:
if (usbReqAppHandler) {
processAppRequest(data); // Forward request to the application thread
} else {
setRequestResult(req, USB_REQUEST_RESULT_ERROR);
}
return;

This comment has been minimized.

Copy link
@sergeuz

sergeuz May 8, 2017

Member

Same here:

if (usbReqAppHandler) {
  processAppRequest(data);
  return;
} else {
  result = USB_REQUEST_RESULT_ERROR;
}
@avtolstoy

This comment has been minimized.

Copy link
Member Author

commented May 9, 2017

Thanks for the comments! Updated PR.

@sergeuz
sergeuz approved these changes May 9, 2017

@technobly technobly added the internal label May 9, 2017

@technobly technobly merged commit 0a9e4aa into feature/photon/wiced-3.7.0-7 May 9, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@technobly technobly deleted the feature/usb-vendor-request-sync branch May 9, 2017

avtolstoy added a commit to avtolstoy/firmware that referenced this pull request May 10, 2017
Merge pull request particle-iot#1323 from spark/feature/usb-vendor-re…
…quest-sync

Process USB vendor requests in system thread
avtolstoy pushed a commit to avtolstoy/firmware that referenced this pull request May 10, 2017
Merge pull request particle-iot#1323 from spark/feature/usb-vendor-re…
…quest-sync

Process USB vendor requests in system thread
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.