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

[UDP] replace UDP offsets by struct UdpHeader #356

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ArnaudD-FR
Copy link
Contributor

Replace UDP array indexes by struct UdpHeader

Description

  • less error prone

  • decrease code size to flash (save some bytes, see below)

  • move ethernet_header(), ethernet_payload(), ip_header(),
    ip_payload().... basic functions to new header EtherUtils.h as those function are required from many places. I didn't want to make them "public" by moving them to EtherCard.h, that's why there is a new header

  • adapt htons to void htons(uint16_t &d, const uint16_t v). I've not been able to reduce ASM size of uint16_t htons(const uint16_t v), so I've created a new one accepting the returned value as reference and reduce the code size for each call by 6 bytes.

Rework of htons

for example (building getDHCPandDNS, using CC_VERSION = 5.4.0 (avr-gcc)):

using uint16_t htons(const uint16_t v)

void EtherCard::udpTransmit (uint16_t datalen) {
    IpHeader &iph = ip_header();
    iph.totalLen = htons(sizeof(IpHeader) + sizeof(UdpHeader) + datalen);
    fill_ip_hdr_checksum(iph);
   ...
}

generate the following code:

00000960 <_ZN9EtherCard11udpTransmitEj>:
    0f 93           push    r16
    1f 93           push    r17
    cf 93           push    r28
    df 93           push    r29
    ec 01           movw    r28, r24
    0b ec           ldi r16, 0xCB   ; 203
    12 e0           ldi r17, 0x02   ; 2
    4c 96           adiw    r24, 0x1c   ; 28
    98 27           eor r25, r24
    89 27           eor r24, r25
    98 27           eor r25, r24
    f8 01           movw    r30, r16
    91 8b           std Z+17, r25   ; 0x11
    80 8b           std Z+16, r24   ; 0x10
    89 ed           ldi r24, 0xD9   ; 217
    92 e0           ldi r25, 0x02   ; 2
    0e 94 98 02     call    0x530   ; 0x530 <_ZL20fill_ip_hdr_checksumR8IpHeader.lto_priv.53>

using void htons(uint16_t &d, const uint16_t v)

void EtherCard::udpTransmit (uint16_t datalen) {
    IpHeader &iph = ip_header();
    htons(iph.totalLen, sizeof(IpHeader) + sizeof(UdpHeader) + datalen);
    fill_ip_hdr_checksum(iph);
   ...
}

generate the following code:

<_ZN9EtherCard11udpTransmitEj>:
   0f 93           push    r16
   1f 93           push    r17
   cf 93           push    r28
   df 93           push    r29
   ec 01           movw    r28, r24
   4c 96           adiw    r24, 0x1c   ; 28
   0b ec           ldi r16, 0xCB   ; 203
   12 e0           ldi r17, 0x02   ; 2
   f8 01           movw    r30, r16
   90 8b           std Z+16, r25   ; 0x10
   81 8b           std Z+17, r24   ; 0x11
   89 ed           ldi r24, 0xD9   ; 217
   92 e0           ldi r25, 0x02   ; 2
   0e 94 98 02     call    0x530   ; 0x530 <_ZL20fill_ip_hdr_checksumR8IpHeader.lto_priv.53>

as we can see the following 3 lines doing the bytes swap have disappeared for the same result:

    98 27           eor r25, r24
    89 27           eor r24, r25
    98 27           eor r25, r24

Code size evolution depending on examples

Differences are between revisions 7f42021 and 5ca0418

Negative value: save size
Positive value: increase size

test old program size old data size new program size new data size program size delta data size delta
backSoon 8100 939 8050 939 -50 0
getDHCPandDNS 10326 1246 10302 1246 -24 0
getStaticIP 9674 827 9624 827 -50 0
getViaDNS 10168 820 10144 820 -24 0
multipacket 7730 1279 7682 1279 -48 0
nanether 8020 1093 7982 1093 -38 0
noipClient 14666 1355 14644 1357 -22 2
notifyMyAndroid 11204 1656 11180 1656 -24 0
ntpClient 8984 865 8932 865 -52 0
persistence 10838 823 10814 823 -24 0
pings 10054 1146 10034 1146 -20 0
rbbb_server 8522 881 8470 881 -52 0
SSDP 9220 1153 9138 1153 -82 0
stashTest 7152 539 7152 539 0 0
testDHCP 7808 1101 7788 1101 -20 0
testDHCPOptions 7948 1113 7928 1113 -20 0
thingspeak 14744 1380 14722 1380 -22 0
twitter 11086 1386 11062 1386 -24 0
udpClientSendOnly 8426 1156 8386 1156 -40 0
udpListener 8376 964 8302 964 -74 0
webClient 10314 1164 10290 1164 -24 0
xively 14806 1058 14782 1058 -24 0

uint16_t dport;
uint16_t length;
uint16_t checksum;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This struct should also be __attribute__((__packed__)), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for this attribute, this struct is perfectly aligned even for 32 bits microcontrollers

Arnaud added 3 commits February 11, 2019 20:52
 * keep MAC/IP relations into an ARP 'store'.
 * fix issue when IP packet is received from local network: answer is
   no more broadcasted
 * define ETHERCARD_ARP_STORE_SIZE to set cache size
 * less error prone

 * decrease code size to flash

 * introduce ethernet_header(), ethernet_payload(), ip_header(),
   ip_payload().... basic functions to retrieve headers and payloads

 * use ntoh and hton instead of manual multibytes adaptations.
   getBigEndianLong has been replaced by standard function ntohl.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants