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

support ether:begin to work with mac placed in flash #398

Closed
wants to merge 4 commits into from
Closed

support ether:begin to work with mac placed in flash #398

wants to merge 4 commits into from

Conversation

nagimov
Copy link

@nagimov nagimov commented Aug 14, 2020

This is a replacement to PR #397 supporting EtherCard:begin to work with mac-addresses placed in flash via PROGMEM.
Since the signature of existing EtherCard:begin implementation expects mac as const uint8_t* macaddr and captures PROGMEM arrays, I think it is easier to supply an extra bool rather than reimplement it with const __FlashStringHelper*.

EtherCard/src/EtherCard.h

Lines 130 to 131 in 420dd84

static uint8_t begin (const uint16_t size, const uint8_t* macaddr,
uint8_t csPin = SS);

I surely could be missing an obvious better way to do this.

Tested on nano v3 with old bootloader.

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.

Having an optional argument telling whether to read from RAM is a lot more error prone than overloading the method to properly handle it automatically.
That being said, I think this optional bool argument approach is used somewhere else in the code too.

These are my two cents

@@ -128,7 +128,7 @@ class EtherCard : public Ethernet {
* @return <i>uint8_t</i> Firmware version or zero on failure.
Copy link
Contributor

Choose a reason for hiding this comment

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

update docs here

copyMac(mymac, macaddr);
}
else {
Serial.println("123456");
Copy link
Contributor

Choose a reason for hiding this comment

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

remove debug print

@nuno-silva
Copy link
Contributor

Also, if you're defaulting to macFromRam=false, this means that most of the examples (e.g. etherNode, nanether, ...) will need to be updated accordingly. Hence another reason why overloading would be better.

@nagimov
Copy link
Author

nagimov commented Aug 14, 2020

@nuno-silva
I agree that "smarter" EtherCard:begin() is a better approach. I'm not confident enough in my cpp to do such a rewrite though :) I'm happy to test a better version if someone could do the rewrite, I have Uno and Nano v3.
I went with optional bool since dhcpSetup and dnsLookup also use it:

static bool dhcpSetup (const char *hname = NULL, bool fromRam =false);

static bool dnsLookup (const char* name, bool fromRam =false);

You're right regarding examples - many of them place mymac into SRAM apparently:

$ find ./examples -type f -exec grep -l "static byte mymac\[\] = " {} \;
./examples/testDHCPOptions/testDHCPOptions.ino
./examples/backSoon/backSoon.ino
./examples/pings/pings.ino
./examples/persistence/persistence.ino
./examples/xively/xively.ino
./examples/multipacketSD/multipacketSD.ino
./examples/webClient/webClient.ino
./examples/getDHCPandDNS/getDHCPandDNS.ino
./examples/udpClientSendOnly/udpClientSendOnly.ino
./examples/multipacket/multipacket.ino
./examples/testDHCP/testDHCP.ino
./examples/rbbb_server/rbbb_server.ino
./examples/thingspeak/thingspeak.ino
./examples/udpListener/udpListener.ino
./examples/SSDP/SSDP.ino
./examples/JeeUdp/JeeUdp.ino
./examples/noipClient/noipClient.ino
./examples/etherNode/etherNode.ino
./examples/getStaticIP/getStaticIP.ino
./examples/getViaDNS/getViaDNS.ino
./examples/notifyMyAndroid/notifyMyAndroid.ino

I'll add another commit updating all the examples. However seeing how many of them (and potentially existing code) are going to get affected, I'm not sure anymore whether this PR should be merged 👎

@nuno-silva
Copy link
Contributor

@nagimov tell you what: I'll implement it as I suggested when I get a chance (maybe tomorrow) and then you can test it for me :)

nuno-silva added a commit to nuno-silva/ethercard that referenced this pull request Aug 16, 2020
nuno-silva added a commit to nuno-silva/ethercard that referenced this pull request Nov 22, 2020
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 nagimov closed this Nov 26, 2020
@nagimov
Copy link
Author

nagimov commented Nov 26, 2020

fixed by #406

@nagimov nagimov deleted the support-mac-from-flash branch November 26, 2020 23:44
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