Custom dhcp options

[bwjohns4]

Re-write of the PR #8451 which adds class member functions to add custom DHCP options. There is function called add_dhcps_custom_options() which adds a DHCP option to a char[100] array. Everytime add_dhcps_custom_options() is called, the new option is appended to the char[100] array. There is also a remove_dhcps_custom_options() which clears all the options from the char[100] array.

Actually meant for this to be a DRAFT PR. I left #include "Arduino.h" and Serial.println's in there to assist with debugging, etc.

[d-a-v]

Thanks!
There are numbers of comments, sorry :]

More generally, you made your former #8451 more complex and ram consuming
by storing user data in a temporary place which is always / not dynamically allocated.

This comment I made was a proposal to keep with the initial simplicity of your original pull request, but with adding the same checks that you wrote several times in this pull request.

The benefit of the former one is simplicity and much more reducing ram consumption.

[d-a-v]

Constants are generally forbidden in any code.
In this case 100 must be replaced by sizeof(dhcpCustomOffers).

[d-a-v]

Here and below, debug code must be optional with #if DHCPS_DEBUG barriers and be using os_printf(),
like in other places in this file.

[d-a-v]

This name is misleading: sizeof generally means a constant which is the size of a type.
This name may be changed to lengthOfCustomOptions.

[d-a-v]

You may simply want to use strcpy_P(&dhcpCustomOffers[sizeOfCustomOptions + 1], offerContent); ?
(using _P so user input can be in flash with PROGMEM / PSTR)

[d-a-v]

using int8 or int16 generates more code than int on esp8266.
if 16 is important but not critical, we have int_fast16_t.
https://en.cppreference.com/w/c/types/integer

const char* is always preferred than char*

A comment may mention that the first bytes indicates the option length.

[d-a-v]

memset(dhcpCustomOffers, 0, sizeof(dhcpCustomOffers)) which is libc-optimized ?

[d-a-v]

casting to size_t is more efficient (casting to 16 or 8 bits produces larger code).

[d-a-v]

strlen(offerContent) seems to be invalid, since this is not a regular C-string.
Should it be offerContent[0] instead ?
(= length of the DHCP option)

[d-a-v]

memcpy(optptr, &dhcpCustomOffers[i+1], dhcpCustomOffers[i+1] + 1) ?

[bwjohns4]

@d-a-v , Thanks so much for the detailed comments! You've got a great point that weak function was much simpler and resulted in less RAM for 99% of cases where this will not be used. My only thought was that it was less clear and inviting to only provide the weak function (users would really have to look closely to see this as an option), where adding the member functions makes it stand out a bit more that this functionality is available. This leads me to two questions:

1. Should I revert back to the original PR and just go with the improvements suggested to that and keep it simple since most users won't need this.
2. OR, simplify this newer version and require the memory allocation to be done by the user and pointer passed per your suggestion. I think the tradeoff would be that the functions would always exist in memory in this PR but allow more clear features to users, whereas with the old PR, the additional memory would only be consumed if the weak function was overridden at compile time by certain users (however the features would be less obviously available).

I'm happy to proceed either way!

[bwjohns4]

@d-a-v , I revised this to simplify and allow just passing a pointer to the user custom options. Your thoughts? Should I continue down this path and potentially improve/optimize or revert back to the weak function option?

[mcspr]

5c: option content is u8, not string or char. yes, we have unsigned chars, but it is a different type from the compiler pov nonetheless. plus, like you said - if you struggle to find api user for you code, just use it internally where the options are applied?

[bwjohns4]

@mcspr From user code, I am able to successfully get this to work by calling dhcpSoftAP.add_dhcps_custom_options("\x72\x0eThis is a test");

Are you saying that I should change void DhcpServer::add_dhcps_custom_options(char *offerContent) to void DhcpServer::add_dhcps_custom_options(uint8_t *offerContent)

[mcspr]

I am suggesting to take a look at existing options prepended inside of the class, and reference RFCs related to options and DHCP itself.
We know what the option is, API might as well have something structured to work with
[code, one byte] [length, one byte] [payload, whatever was in length (limited to 255 overall)]?

Can we write it in a way useful for both? What happens if you get length wrong with the char string encoding? Why not something like vector instead of a fixed-size storage (struct Option { ... }, vector, etc.)?

Also note that any option may appear only once https://datatracker.ietf.org/doc/html/rfc2131#section-4.1

[bwjohns4]

@mcspr , I feel like I'm not sure where to go with this at this point. I could add more code to validate the custom user options by checking that each option is added only once and iterating through the user provided options to check them. But this would add a lot more complexity to this PR. Should we just leave it as it is now (it works) and allow the user to ensure correct options when/if they choose to use this feature? I don't mind adding the complexity to validate this, I just don't want to bloat the codebase beyond what's needed. Your thoughts?

[mcspr]

Let me write up my proposal as a patch, then. I feel like my comment was not clear enough.
edit: #8582

[mcspr]

superseded by #8582