Closed fubian closed 3 years ago
Seems that replacing tabs by spaces is a good idea, that file has mixed tabs and spaces which is hardly ever a good idea.
The coding style is inconsistent anyways: compare the following examples.
Seems you looked further than I did, so fine to leave it like this.
The following snipped from line 668 to line 712 illustrates in the last else clause that the two consecutive UDD_Send8(EP0, 0); commands shall be within the same scope (line 708 and line 709).
I'm not so sure I follow this. Do you mean because there are also two UDD_Send8
in the ENDPOINTHALT
case, so you expect to be sending two bytes rather than one? If so, that argument actually argues against putting both sends in the else. You now have this: https://github.com/fubian/ArduinoCore-sam/blob/50ee941415f6c39f31972f5259ba75feaea16f11/cores/arduino/USB/USBCore.cpp#L703-L711
However, that sends 1 or 2 bytes, depending on the value of isRemoteWakeupEnabled
, which I'm not sure is correct. To really know what is correct, the USB spec should be consulted.
So I did (I looked at the main USB spec, version 2.0, April 27, 2000). For GET_STATUS
, it should always return 2 bytes, as indicated by the wLength
field:
The returned values for the case handled here, are indeed just a halt bit:
So, your latest push seems incorrect, since it sometimes returns just one byte.
For CLEAR_FEATURE
, something weird is going on. The specification says that no bytes should be returned, as indicated by a wLength of 0:
Maybe I'm misunderstanding what UDD_Send8
means exactly, but it seems like CLEAR_FEATURE
is not being handled properly (though maybe some lower level handling checks the wLength
field and just silently drops these 2 bytes, dunno). Also, it seems that the isRemoteWakeupEnabled
field is not actually cleared. It can be set, but not queried and is also not actually used anywhere, so I guess this is just unfinished implementation that we can just ignore at least in the scope of this PR.
Finally, as for the structure of your commits, you have now mixed a functional change (moving UDD_Send8
calls into else clauses) with a very big mechanical commit (replacing tabs with spaces), which makes it pretty much impossible to spot the difference. These changes should really have been be separated (though it seems the moving into the else should be reverted in any case, so that might not be so much of an issue).
Ideally, you would get two commits:
This requires some git history editing (e.g. with rebase) and a force push (please do not open a new PR for this). Let me know if you can't figure this out (git is not the most friendly out there), then I can also fix this for you.
Ok. Thanks for your detailed review! I sorted my commits and corrected the UDD_Send8(EP0, 0)
issue of my previous suggestions.
The coding style is inconsistent anyways: compare the following examples.
if
clause in line 250if
clause in line 287if
clause in line 398I replaced now all the tabs with four spaces.
The following snipped from line 668 to line 712 illustrates in the last
else
clause that the two consecutiveUDD_Send8(EP0, 0);
commands shall be within the same scope (line 708 and line 709).Due to that, I corrected the placement of the braces.