-
Notifications
You must be signed in to change notification settings - Fork 19
Feature/reorganise library #122
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
base: develop
Are you sure you want to change the base?
Feature/reorganise library #122
Conversation
a12a712 to
b5e161a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 59 out of 96 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3ae7385 to
a6a3aec
Compare
xross
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not part of the changes in this PR, but looks to be quite a few generically named header files and defines which might be inconvenient if/when they clash with other items.
| size_t len_ep0 = 0; | ||
|
|
||
| XUD_Result_t loop_result = XUD_RES_OKAY; | ||
| while ((loop_result == XUD_RES_OKAY) && (len_ep0 < sp->wLength)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this loop not ideal, assumes host will send the amount of data it said it would. Should be loop while received packet size == max packet size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be checking for bus resets too I guess - for example, a badly timed unplug is going to break this protocol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be checking for bus resets too I guess - for example, a badly timed unplug is going to break this protocol
Well as far as I understand thats why it checks for (loop_result == XUD_RES_OKAY), so any status changes reported can be handled. Am I wrong here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this loop not ideal, assumes host will send the amount of data it said it would. Should be loop while received packet size == max packet size.
Please clarify? Whats the case you are trying to catch?
So the the cases I see are:
- host sends to much data, the loop has already exited and the host will wait and then timeout, can this happen?
- host sends too little data, waits on respond and times out, and probably a bus reset, so returns XUA_RES_UPDATE,
What am I missing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be checking for bus resets too I guess - for example, a badly timed unplug is going to break this protocol
Well as far as I understand thats why it checks for
(loop_result == XUD_RES_OKAY), so any status changes reported can be handled. Am I wrong here?
Sorry, yes, missed that. Any bus events should be handler by the calling function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this loop not ideal, assumes host will send the amount of data it said it would. Should be loop while received packet size == max packet size.
Please clarify? Whats the case you are trying to catch?
So the the cases I see are:
- host sends to much data, the loop has already exited and the host will wait and then timeout, can this happen?
- host sends too little data, waits on respond and times out, and probably a bus reset, so returns XUA_RES_UPDATE,
What am I missing here?
Its the second bullet from the spec I'm trying to capture (5.5.3 Control Transfer Packet Size Constraints)
The Data stage of a control transfer from an endpoint to the host is complete when the endpoint does one of
the following:
• Has transferred exactly the amount of data specified during the Setup stage
• Transfers a packet with a payload size less than wMaxPacketSize or transfers a zero-length packet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note, this also catches the "too much" data case you mention (just need to guard not overrunning a buffer in this case..)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So do we need to test for, which I think to mentioned, recLength != wMaxPacketSize?
Should be loop while received packet size == max packet size.
Was you comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah so continue to receive packets whilst recLength == wMaxPacketsize
If you get a packet size < wMaxPacketSize (including the 0 length case) then exit loop.
Im surprised there isn't such a receive loop function in lib_xud, looks like we only have the sending case covered there.
xross
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spotted a couple of things, the usb protocol I think is a debug session waiting to happen ;)
ed-xmos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice update and cleanup. Some minor comments - I think Ross's ones are more "request changes" than mine which are mostly advisories/questions
c052803 to
1dba13b
Compare
Adding cmake to aid build host apps
1dba13b to
e24e7cf
Compare
Building host apps would require a lot of copy-paste, adding cmakes for variants to assist building specific host apps.
Plus,
devicefolder,