njh / EtherCard

EtherCard is an IPv4 driver for the ENC28J60 chip, compatible with Arduino IDE
https://www.aelius.com/njh/ethercard/
GNU General Public License v2.0
1.03k stars 455 forks source link

read MAC address from PROGMEM #406

Open nuno-silva opened 3 years ago

nuno-silva commented 3 years ago

This fixes #397 by adding support for reading the MAC address from PROGMEM.

I wanted to do it using an overloaded begin() method, but it turns out that it's not possible to do that using the PROGMEM macro (see https://github.com/njh/EtherCard/pull/399#issuecomment-731627907) :disappointed: . Therefore, I implemented it using an optional fromRam argument, as is done in other places. Despite having updated all examples to read from PROGMEM, the fromRam argument is kept as true by default so as not to break other people's code when they update the library. This does have the downside that it's not possible to place the MAC in PROGMEM without passing a csPin to begin().

@nagimov It's been a few months but I'd be grateful if you could still test this :)

closes #399 closes #398 fixes #397 (ntpClient example)

nagimov commented 3 years ago

Hey @nuno-silva thanks for finishing this! :+1:

Just tested on Nano V3 with optiboot using ntpClient example. Both cases with fromRam=true and fromRam=false run ok. RAM & FLASH utilization change as expected. MAC is reported to DHCP correctly.

nagimov@thinkpad:~$ # install patched version
nagimov@thinkpad:~$ cd ~/Arduino/libraries
nagimov@thinkpad:~/Arduino/libraries$ mv EtherCard EtherCard.bak
nagimov@thinkpad:~/Arduino/libraries$ git clone https://github.com/nuno-silva/ethercard
Cloning into 'ethercard'...
remote: Enumerating objects: 75, done.
remote: Counting objects: 100% (75/75), done.
remote: Compressing objects: 100% (47/47), done.
remote: Total 1974 (delta 15), reused 36 (delta 4), pack-reused 1899
Receiving objects: 100% (1974/1974), 806.49 KiB | 14.93 MiB/s, done.
Resolving deltas: 100% (1027/1027), done.
nagimov@thinkpad:~/Arduino/libraries$ cd ethercard
nagimov@thinkpad:~/Arduino/libraries/ethercard$ git checkout begin-progmem
Branch 'begin-progmem' set up to track remote branch 'begin-progmem' from 'origin'.
Switched to a new branch 'begin-progmem'
nagimov@thinkpad:~/Arduino/libraries/ethercard$ git log --pretty=oneline -2
3b6776215f99e6fbe300a51e43d647e21fc02dba (HEAD -> begin-progmem, origin/begin-progmem) update examples to keep MAC in PROGMEM instead of RAM
c2ba42015efdd85cbe50e4634ef4e8203d5a45b4 EtherCard::begin(): support reading MAC address from PROGMEM
nagimov@thinkpad:~/Arduino/libraries/ethercard$ cd examples/ntpClient
nagimov@thinkpad:~/Arduino/libraries/ethercard/examples/ntpClient$ 
nagimov@thinkpad:~/Arduino/libraries/ethercard/examples/ntpClient$ # test with MAC in FLASH
nagimov@thinkpad:~/Arduino/libraries/ethercard/examples/ntpClient$ arduino-cli compile --fqbn arduino:avr:nano:cpu=atmega328 ./
Sketch uses 8796 bytes (28%) of program storage space. Maximum is 30720 bytes.
Global variables use 829 bytes (40%) of dynamic memory, leaving 1219 bytes for local variables. Maximum is 2048 bytes.
nagimov@thinkpad:~/Arduino/libraries/ethercard/examples/ntpClient$ arduino-cli upload --port /dev/ttyUSB0 --fqbn arduino:avr:nano:cpu=atmega328
nagimov@thinkpad:~/Arduino/libraries/ethercard/examples/ntpClient$ screen /dev/ttyUSB0 9600
[EtherCard NTP Client]
IP:  192.168.50.159
GW:  192.168.50.1
DNS: 192.168.50.1
NTP: 213.136.12.53
Started listening for response.
NTP request sent.
NTP response received.
Unix time = 1606432870

[screen is terminating]
nagimov@thinkpad:~/Arduino/libraries/ethercard/examples/ntpClient$ 
nagimov@thinkpad:~/Arduino/libraries/ethercard/examples/ntpClient$ # test with MAC in SRAM
nagimov@thinkpad:~/Arduino/libraries/ethercard/examples/ntpClient$ sed -i 's/ether.begin(sizeof Ethernet::buffer, myMac, SS, false)/ether.begin(sizeof Ethernet::buffer, myMac, SS, true)/g' ntpClient.ino
nagimov@thinkpad:~/Arduino/libraries/ethercard/examples/ntpClient$ sed -i 's/const byte myMac\[\] PROGMEM/const byte myMac\[\]/g' ntpClient.ino
nagimov@thinkpad:~/Arduino/libraries/ethercard/examples/ntpClient$ sed -i 's/0x70, 0x69, 0x69, 0x2D, 0x30, 0x31/0x70, 0x69, 0x69, 0x2D, 0x30, 0x32/g' ntpClient.ino
nagimov@thinkpad:~/Arduino/libraries/ethercard/examples/ntpClient$ arduino-cli compile --fqbn arduino:avr:nano:cpu=atmega328 ./
Sketch uses 8792 bytes (28%) of program storage space. Maximum is 30720 bytes.
Global variables use 835 bytes (40%) of dynamic memory, leaving 1213 bytes for local variables. Maximum is 2048 bytes.
nagimov@thinkpad:~/Arduino/libraries/ethercard/examples/ntpClient$ arduino-cli upload --port /dev/ttyUSB0 --fqbn arduino:avr:nano:cpu=atmega328
nagimov@thinkpad:~/Arduino/libraries/ethercard/examples/ntpClient$ screen /dev/ttyUSB0 9600
[EtherCard NTP Client]
IP:  192.168.50.160
GW:  192.168.50.1
DNS: 192.168.50.1
NTP: 213.136.12.54
Started listening for response.
NTP request sent.
NTP response received.
Unix time = 1606433017

[screen is terminating]

Logs from dhcpd:

pi@raspberrypi:~ $ cat /var/log/syslog | grep DHCPOFFER | tail -4
Nov 26 23:21:08 raspberrypi dnsmasq-dhcp[522]: DHCPOFFER(eth0) 192.168.50.159 70:69:69:2d:30:31 
Nov 26 23:21:10 raspberrypi dnsmasq-dhcp[522]: DHCPOFFER(eth0) 192.168.50.159 70:69:69:2d:30:31 
Nov 26 23:23:37 raspberrypi dnsmasq-dhcp[522]: DHCPOFFER(eth0) 192.168.50.160 70:69:69:2d:30:32 
Nov 26 23:23:37 raspberrypi dnsmasq-dhcp[522]: DHCPOFFER(eth0) 192.168.50.160 70:69:69:2d:30:32
nuno-silva commented 3 years ago

Awesome! Thanks for testing @nagimov !

nuno-silva commented 3 years ago

@njh please review and merge :)

nuno-silva commented 3 years ago

Hey @njh ! Any chance of this being merged? :)

nuno-silva commented 3 years ago

@njh thanks for looking into this. Unfortunately it seems TravisCI is not running:

Could not authorize build request for njh/EtherCard.

Maybe you need to login to Travis and re-authorize the repo?

njh commented 3 years ago

Sorry for taking so long to look at this @nuno-silva 😞 I have sorted Travis out now.

Part of the delay in reviewing this PR is that I would like to see if I can get the __FlashStringHelper class to work with the F() macro and the MAC address in a programme memory array.

Alternatively, I could port over my MACAddress class from EtherSia 🤔

nuno-silva commented 3 years ago

I tried using PROGMEM in #399, but unfortunately I wasn't able to make it work. See https://github.com/njh/EtherCard/pull/399#issuecomment-731627907 . Maybe using __FlashStringHelper would work, I'm not sure.

njh commented 3 years ago

Sorry it has taken me so long to respond to this.

I am not very keen on the fromRam argument for a couple of reasons:

  1. If you have a conditional statement, then the code is always included in the program - it can't be stripped by the linker, if it isn't used. Whereas if you use different functions, or overload functions, then the unused code can be stripped.
  2. Boolean parameters makes code harder to read

I have done some experiments with passing a MAC address stored in program memory into a function: https://gist.github.com/njh/de1cec7c883bcfe35df3956340fa30ab

The F() macro can be used to define a inline string in program memory. It does work to use it like this to pass in a MAC address, but it is a bit ugly:

ether.begin(sizeof Ethernet::buffer, F("\x74\x69\x69\x2D\x30\x31"));

The thing that is missing is the FPSTR() macro. This is defined in WString.h for the ESP architecture core, but not in the WString.h for avr core. But we could put it into EtherCard.h. And I might try raising a Pull Request and see if it is accepted in ArduinoCore-avr.

This would allow you to do:

const static byte mymac[] PROGMEM = { 0x74, 0x69, 0x69, 0x2D, 0x30, 0x31 };

ether.begin(sizeof Ethernet::buffer, FPSTR(mymac));

@nuno-silva What do you think? Does combining the PROGMEM attribute with the FPSTR() macro seem reasonable to you?

nuno-silva commented 3 years ago

Hi @njh! No problem!

I agree that the fromRam argument is not the best solution and the FPSTR() macro does seem like a good idea :)