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

Add ARP cache #351

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add ARP cache #351

wants to merge 1 commit into from

Conversation

ArnaudD-FR
Copy link
Contributor

Pros:

Cons:

  • increase size of Ethercard in my project:
    • +198 bytes in Program section (.text + .data + .bootloader)
    • +27 bytes in Data section (.data + .bss + .noinit)

About issue #269 the following code should be used:

#define PORT 5000

#include <EtherCard.h>
#include <IPAddress.h>

uint8_t macAddr[] = {0x74,0x69,0x69,0x2D,0x30,0x32};
uint8_t ipAddr[] = { 192, 168, 1, 200 };
uint8_t gwAddr[] = { 192, 168, 1, 2 };
uint8_t netMask[] = { 255, 255, 255, 0 };

uint8_t distAddr[] = { 192, 168, 1, 52 }; // I can't send packet to 172.16.255.2.

byte Ethernet::buffer[256];

void setup() {
    Serial.begin(57600);
    ether.begin(sizeof(Ethernet::buffer), macAddr);
    ether.staticSetup(ipAddr, gwAddr, 0, netMask);

    // wait for link to be up before any ARP lookup
    while (!ether.isLinkUp())
        ;

    // send ARP lookup...
    ether.clientResolveIp(distAddr);
    // ...and wait for answer
    while (ether.clientWaitIp(distAddr))
        ether.packetLoop(ether.packetReceive());
    Serial.println("setup done");
}

void loop() {
    char c[] = { 'h', 'i' };
    ether.sendUdp(c, sizeof(c), PORT, distAddr, PORT);
    ether.packetLoop(ether.packetReceive());
    delay(1000);
}

@nuno-silva
Copy link
Contributor

@ArnaudD-FR this is a good feature! However, it would be nice if one could disable ARP cache completely to save some RAM and program memory, if needed. Useful for memory-constrained setups where broadcasting would be fine. I suggest adding a pre-processor macro like DISABLE_ARP_CACHE (commented out by default).

@ArnaudD-FR
Copy link
Contributor Author

I understand your concern but this implementation replace the previous mechanism to store MAC address for gateway, dnsip and hisip. So if we disable the ARP cache, there is no way to communicate.

Do you have some use case in such environment to determine the best implementation?

  • communication is done only with one device (gateway or whatever?)
  • ...

@njh
Copy link
Owner

njh commented Jan 7, 2019

Wow, that is quite a piece of work, thanks ArnaudD-FR.

It changes quite a lot of stuff in this changeset- not just adding ARP cache - such as new IP header structures.

I agree with @nuno-silva that RAM is one of the scarcest commodities, particularly on the older AVR processors. This is probably the reason why there wasn't an ARP cache in the first place.

Rather than disabling the ARP cache, what would happen if it only had a single entry (for the router)?

@ArnaudD-FR
Copy link
Contributor Author

ArnaudD-FR commented Jan 7, 2019

The RAM usage is the following struct size multiplied by the number of ARP cache entries: (IP_LEN + ETH_LEN + 1)*ETHERCARD_ARP_STORE_SIZE, so by default it is 44 bytes.

struct ArpEntry
{
    uint8_t ip[IP_LEN];
    uint8_t mac[ETH_LEN];
    uint8_t count;
};

But my patch remove some global variables (17 bytes) from tcpip.cpp:

  • uint8_t destmacaddr[ETH_LEN]
  • boolean waiting_for_dns_mac
  • boolean has_dns_mac
  • boolean waiting_for_dest_mac
  • boolean has_dest_mac
  • uint8_t gwmacaddr[ETH_LEN]
  • uint8_t waitgwmac

So the RAM increase is about 27 bytes as defined in my first message when using the default cache size. If you set the cache to 1 entry then you save 6 bytes... But only the gateway can be contacted. If something else contact the device, the gateway MAC address will be replaced by the last one received and on next packetLoop the gateway MAC address will come back...

Yes I really don't like the idea to use array index to find ethernet/arp/ip header fields. It's so much easier to use and understand a structure...

The IP header definition is here just to get the IP address of an IP packet and update the corresponding ARP entry.

In this commit I have completely replaced the ARP indexes by a struct ArpHeader.

Copy link
Contributor

@nuno-silva nuno-silva left a comment

Choose a reason for hiding this comment

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

Thank you for your work!

#define ETH_ARP_OPCODE_REPLY HTONS(0x0002)
#define ETH_ARP_OPCODE_REQ HTONS(0x0001)

struct ArpHeader
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.

If I don't make mistakes all supported AVR are 8bits aligned, so adding this attribute won't change anything.

If this library support 16/32bits chipsets the EthHeader should have this attribute. Is it the case?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so, but there's no harm in adding it for future proof :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Be careful with this attribute, it's not always harmless, you can generate some "bus error" when data are not aligned with some architectures (like MIPS for example).

All those structures are correctly aligned by default, so I prefer to not add this attribute, it is useless in this case, even with EthHeader. I've just checked on amd64 and this code:

#include <iostream>

struct EthHeader
{
    uint8_t target[6];
    uint8_t source[6];
    uint16_t    etype;
};

int main()
{
    std::cout << "sizeof(EthHeader) = " << sizeof(EthHeader) << std::endl;
    return 0;
}

output: sizeof(EthHeader) = 14

So struct even EthHeader is fine.

 * 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
Copy link

@bcsanches bcsanches left a comment

Choose a reason for hiding this comment

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

Hi folks

my udp network code on lan was not working and this fixed my issue. So it "works for me".

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.

4 participants