Get USBHost to build and run with GCC_ARM #67
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I updated a few things in the USBHost source code to get it to compile
with GCC:
* In USBHALHost.cpp, the usb_buf global variable was defined with two
different aligmnent specifiers:
static volatile __align(256) uint8_t usb_buf[TOTAL_SIZE] __attribute((section("AHBSRAM1"),aligned)); //256 bytes aligned!
The first one was not accepted by GCC. I removed the duplicate
alignment specifier and updated the one in the existing __attribute
property to force the desired 256 byte alignment.
* Removed the explicit use of the __packed property from structures
and instead used the PACKED macro defined in the mbed SDK's
toolchain.h header file. This macro does the right thing for the
various compilers supported by the mbed team.
* Updated USB_* message macros in dbg.h to place spaces around
references to the 'x' macro parameter. Without this, C++ 11
compilation thought the x character was a string literal operator but
it wasn't one it recognized. Adding the spaces makes it easier to
see the use of the parameter, fixes this compile time error, and
doesn't add any extra space to the final output string since the
compiler only concatenates the contents within the double quotes of
the strings.
By the way, I build with the gnu++11 standard since I have had requests
for its support from gcc4mbed. Some of the items fixed here might not
be errors when using older standards with GCC.
I built and tested a USBHostMSD sample with these updates using both
GCC and the online compiler. I will test more of the host interfaces
before issuing a pull request containing this commit.
The two following memcpy() calls in USBEndpoint::init() are passing in
a NULL pointer as the copy source location.
memcpy(td_list_[0], 0, sizeof(HCTD));
memcpy(td_list_[1], 0, sizeof(HCTD));
I suspect that these were meant to be memset() calls instead so I
switched them. In one of the places that I found where this method is
called, USBHost::newEndpoint(), its passes in an array of HCTD objects
which have already been cleared with similar memset() calls. I am
therefore pretty certain that these were meant to be memset() calls but
if all callers already guarantee that they are zeroed out then maybe
the memset()s can be removed from USBEndpoint::init() anyway.
There are a few warnings thrown by GCC for this source file that I have
attempted to correct. I would definitely appreciate feedback from
others on these changes:
* USBHost::usb_process() would attempt to write past the end of the
deviceInited[] array in the following code snippet:
if (i == MAX_DEVICE_CONNECTED) {
USB_ERR("Too many device connected!!\r\n");
deviceInited[i] = false;
The i variable is guaranteed to index 1 item past then end of this
array since it only contains MAX_DEVICE_CONNECTED elements. I have
removed the line which sets deviceInited[i] to false. Two questions
result though:
1) What was the intent of this line of code and is it Ok that I
just removed it or should it be replaced with something else?
2) I see no where else that elements in the deviceInited array are
set to false except when all are set to false in the usbThread()
method. Should there be code in DEVICE_DISCONNECTED_EVENT to
do this as well?
* USBHost::transferCompleted(volatile uint32_t addr) was comparing addr
to NULL, which is of pointer type. GCC issues a warning on this
since the types are different (void* being compared to uint32_t). I
switched it to just compare with 0 instead.
* There is a switch statement in USBHost::unqueueEndpoint() which
is conditional on a ENDPOINT_TYPE enumeration but doesn't contain
cases for all values in the enumeration. I just added a default
case to simply break on other values to silence this GCC warning and
allow the code to continue working as it did before. Is it Ok that
this particular piece of code only handles these two particular
cases?
* USBHost::fillControlBuf() was generating a warning about possible
alignment issues when accessing the setupPacket byte array as 16-bit
half words instead. I changed the casting to silence the warnings
and modified the declaration of the setupPacket field to make sure
that it is at least 2-byte aligned for these 16-bit accesses.
I removed the initialization of some variables which were never used to
silence GCC warnings.
One of them had me removing the following line from
USBHostMouse::rxHandler():
int len = int_in->getLengthTransferred();
The variable len is never used again in this method and it doesn't
appear that the int_in->getLengthTransferred() call has any side
effects which would require this extraneous call to remain in the code.
USBHostHub.cpp made a few calls to the USBHALHost::deviceDisconnected() virtual method passing in NULL for the addr parameter which is actually declared as uint32_t and not a pointer type. Switching these calls to pass in a 0 for this parameter silences GCC warnings about incompatible types.
I changed the following initialization from:
uint8_t cmd[6] = {0x12, (lun << 5) | evpd, page_code, 0, 36, 0};
to:
uint8_t cmd[6] = {0x12, uint8_t((lun << 5) | evpd), page_code, 0, 36, 0};
This makes it clear to the compiler that we are Ok with the 32-bit
integer result from the shift and logical OR operation (after integral
promotions) being truncated down to a 8-bit unsigned value. This is
safe as long as lun only has a value of 7 or lower.
Based on great feedback from Martin Kojtal on my previous commit, I
have modified my USBHost::fileControlBuf() change to be more portable.
https://github.com/adamgreen/mbed/commit/ddb3fbe826f800a62c68ae57b7deb7c4777527d8#commitcomment-3988044
The code now fills in the setupPacket byte array a byte at a time,
using bit shifts to extract lower and upper bytes from the 16-bit
values so that the code should work on big endian or little endian
machines. I also removed the 2-byte alignment attribute from the
setupPacket array as it is no longer required.
emilmont
added a commit
that referenced
this pull request
Sep 16, 2013
Get USBHost to build and run with GCC_ARM
artokin
pushed a commit
to artokin/mbed-os
that referenced
this pull request
Aug 23, 2017
… from e125164..d65b6b0 d65b6b0 Update unittests for nsdynmemlib API change (ARMmbed#71) bc69b8b Disable CoAP duplicate message detection (ARMmbed#69) ccb65f8 Change year 2017 to copyright (ARMmbed#68) 76490a7 Add option to join COAP multicast group (ARMmbed#67) 381d910 Register to multicast groups (ARMmbed#66) dce323c Add transaction delete to CoAP service (ARMmbed#65) feea33e Add option to select used socket interface (ARMmbed#63) 5a5c0f9 Merge pull request ARMmbed#62 from ARMmbed/coap_separation 0d26c00 Modifying file headers and Makefile to adapt from libcoap to mbed-coap d323c3a Fixing unit tests based on new coap library d1a3d25 Modifying Makefile and source file based on new coap library git-subtree-dir: features/nanostack/FEATURE_NANOSTACK/coap-service git-subtree-split: d65b6b0
geky
pushed a commit
to geky/mbed
that referenced
this pull request
Aug 25, 2018
FSFAT_SDCARD_INSTALLED - Accepted from mbed_app.json only
linlingao
added a commit
to linlingao/mbed-os
that referenced
this pull request
Jul 12, 2019
Rewrite code to put semaphore in a class
pan-
added a commit
to pan-/mbed
that referenced
this pull request
May 29, 2020
Improve documentation of fix_heartrate_instruction.
pan-
added a commit
to pan-/mbed
that referenced
this pull request
May 29, 2020
[NRF51_DK] Add workaround for make_gcc_arm exporter
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I have made one change to this branch since I originally posted a tracking issue for this item. That change was based on great feedback from Martin. More information about the commits found in this pull request can be found in the individual commit descriptions.
Testing I have performed on these changes:
Thanks for your consideration,
Adam