Create option for setting and getting the hostname #223
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
|
Memory usage change @ 6ad334a
Click for full report table
Click for full report CSV |
gudnimg
reviewed
May 28, 2023
src/Ethernet.cpp
Outdated
Comment on lines
240
to
245
| sprintf(macAddrStr, "%02X", mac[3]); | ||
| strcat(_hostName, macAddrStr); | ||
| sprintf(macAddrStr, "%02X", mac[4]); | ||
| strcat(_hostName, macAddrStr); | ||
| sprintf(macAddrStr, "%02X", mac[5]); | ||
| strcat(_hostName, macAddrStr); |
There was a problem hiding this comment.
Could this be reduced to one line?
sprintf_P(_hostName, PSTR("%02X%02X02X"), mac[3], mac[4], mac[5])
PSTR() saves the format string in program memory (Flash) instead of RAM. I haven't tested my suggestion, just thought it may help shrink code size.
There was a problem hiding this comment.
Good idea. I changed a bit on your suggestion to include the default hostname as well.
Works nicely on my setup. I don't know, how memory usage can be tested. the compiler gives the same information in either case:
"Sketch uses 12230 bytes (39%) of program storage space. Maximum is 30720 bytes.
Global variables use 714 bytes of dynamic memory."
There was a problem hiding this comment.
The report from the Github bot shows change in arduino:avr:mega:
Before:
- Flash memory: +1574 bytes
- SRAM: +28 bytes
After:
- Flash memory: +1482 bytes
- SRAM: +22 bytes
A very welcome change :)
Now I wonder what is adding over 1KB of memory? 🤔 It seems like a lot.
There was a problem hiding this comment.
Could the 1KB increase be explained by the compiler deciding to inline some functions on its own? I've seen that before when LTO is enabled (I'm not sure if that's the case in the default Arduino builds).
There was a problem hiding this comment.
I refactored this again. This time I removed the usage of sprintf, and copied the MAC part on a bit lower level.
Now, I can see much smaller numbers on the "Memory usage change".
Can you check this, please?
gudnimg
reviewed
May 28, 2023
per1234
added
topic: code
|
Memory usage change @ 1f6d782
Click for full report table
Click for full report CSV |
|
Memory usage change @ eb14bb7
Click for full report table
Click for full report CSV |
|
the method name should be |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I implemented a way to set the hostname manually.
I know, there are two similar PRs on this topic:
#50
arduino/Arduino#5701
These are pretty old requests and I haven't seen any valuable updates in the last few years.
I collected all problems and reviews in the other codes and created a solution that fixes the already-mentioned problems.
With this update, you can call
Ethernet.setHostName();andEthernet.getHostName();. If you don't call the setter, the automatically generated hostname will be used, like before. If you call the setter with any text, that will be used and the MAC won't be concatenated.I tried to do my best at the implementation and documentation, but please review this request and let me know, if something needs to be changed.