Update LPC1768 linker script and silence GNU warnings in network stack #31
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
The original script assigned memory ranges to USB_RAM and ETH_RAM but it never placed any section data in those regions. I added clauses towards the bottom of the script to place data that the programmer has marked for the AHBSRAM0 and AHBSRAM1 sections into these regions of RAM. Previously the data destined for these sections was being placed in the lower 32K RAM bank and overflowing it at link time. I also added a few Image$$ linker symbols to mimic those used by the online compiler. I have had samples in the past which took advantage of these to display static memory statistics for each SRAM region. I also changed LENGTH=0x7F38 to LENGTH=(32K - 0xC8) to make it more consistent with the sizing of the other regions in this script which use human readable K sizing information. The 0xC8 subtraction reflects the starting offset of 0xC8 for this region.
The dn variable in lpc_low_level_output() was originally defined as a u32_t but it is later compared to the s32_t return value from lpc_tx_ready(). Since it is intialized to pbuf_clean() which returns a u8_t, a s32_t type can safely hold the initial value and remains consistent with the signed lpc_tx_ready() comparison. I also modifed writtenLen in TCPSocketConnection::send_all() and readLen in TCPSocketConnection::recieve_all() to be of type int instead of size_t. This is more consistent with their usage within these methods (they accumulate int ret values and are compared to the int length value) and their use as a signed integer return values.
GCC will issue a warning when the ip_addr_isany() macro is used on
a pointer which can never be NULL since the macros NULL check will
always be false:
#define ip_addr_isany(addr1) ((addr1) == NULL || \
(addr1)->addr == IPADDR_ANY)
In these cases, it is probably clearer to just perform the
x.addr == IPADDR_ANY check inline.
The first was a potential out of range index read in dhcp_handle_ack().
The (n < DNS_MAX_SERVERS) check should occur first. There is also a
documented lwIP bug for this issue here:
http://savannah.nongnu.org/bugs/?36170
In dhcp_bind() there is no need to perform the NULL check in
ip_addr_isany() for &gw_addr. Just check (gw_addr.addr == IPADDR_ANY)
instead.
I refactored the chaddr[] copy in dhcp_create_msg() to first copy all
of the valid bytes in hwaddr and then pad the rest of the bytes with 0.
Before it used to check on every destination byte if it should copy or
pad. GCC originally complained about an index out of range read from
the hwaddr[] array even though it was protected by a conditional
operator. The refactor makes the intent a bit clearer and saves the
extra comparison per loop iteration. It also stops GCC from
complaining :)
The phy_speed_100mbs, phy_full_duplex, and phy_link_active fields of PHY_STATUS_TYPE are 1 bit wide but lpc_phy_init() attempted to initialize them to a value of 2. I switched the initializations to be 0 instead and it still generated the same .bin image.
|
Hi Adam, Very nice work, thanks a lot for your pull request. Did you manage to run some tests with your modified code? Thanks, |
|
Yes and No. I have built the TCPSocket_HelloWorld sample with GCC ARM and ran it successfully. I have also spent the day debugging performance and robustness issues in a UDP receive application with these changes in place. The linker script change took the code from not even building to building/running. I don't use the Python scripts to build or test though. I can try to get those working on my MacBook tomorrow. |
|
This is enough for now, thank you. |
bogdanm
added a commit
that referenced
this pull request
Aug 15, 2013
Update LPC1768 linker script and silence GNU warnings in network stack
|
Thanks! |
pan-
pushed a commit
to pan-/mbed
that referenced
this pull request
Apr 16, 2018
BLE: Add stub for signing API in Nordic pal security manager.
geky
added a commit
to geky/mbed
that referenced
this pull request
Aug 25, 2018
Rename the test dir (not TESTS) to util
yossi2le
pushed a commit
to yossi2le/mbed-os
that referenced
this pull request
Jan 2, 2019
Incorporate more comprehensive metadata checks
linlingao
pushed a commit
to linlingao/mbed-os
that referenced
this pull request
Jul 12, 2019
UART IRQ Handler Implementation
pan-
pushed a commit
to pan-/mbed
that referenced
this pull request
May 29, 2020
BLE_URLBeacon: Use extern c to import nordic sdk files written in c
pan-
added a commit
to pan-/mbed
that referenced
this pull request
May 29, 2020
[EddystoneObserver] Fix usage of flexible arrays.
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 updated the LPC1768 linker script to place AHBSRAM0 and AHBSRAM1 sections of data into the correct regions of memory. Without these changes, the networking code would try to place all of the lwIP data in the lower 32K and fail to link due to overflow.
I also made a few changes to the networking code to silence various GCC warnings which were emitted at -Wall -Wextra level. I broke these changes down into separate commits and give more description for each of them in their associated commit descriptions.
Please let me know if there is anything you would like me to modify and I can issue a new pull request with the desired modifications.
Thanks,
Adam