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

read MAC address from PROGMEM #406

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

Conversation

nuno-silva
Copy link
Contributor

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 #399 (comment)) 😞 .
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)

Using an optional fromRam argument instead of overloading (see njh#399).
Defaults to fromRam=true so as not to break existing code.

closes njh#399
closes njh#398
@nagimov
Copy link

nagimov commented Nov 26, 2020

Hey @nuno-silva thanks for finishing this! 👍

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
Copy link
Contributor Author

Awesome! Thanks for testing @nagimov !

@nuno-silva nuno-silva marked this pull request as ready for review November 27, 2020 15:24
@nuno-silva
Copy link
Contributor Author

@njh please review and merge :)

@nuno-silva
Copy link
Contributor Author

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

@nuno-silva
Copy link
Contributor Author

@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
Copy link
Owner

njh commented Jul 27, 2021

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
Copy link
Contributor Author

I tried using PROGMEM in #399, but unfortunately I wasn't able to make it work. See #399 (comment) . Maybe using __FlashStringHelper would work, I'm not sure.

@njh
Copy link
Owner

njh commented Aug 23, 2021

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
Copy link
Contributor Author

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 :)

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.

3 participants