patchlwIP - A Lightweight TCP/IP stack - Patches: patch #10194, Make DHCP module more adjustable

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #10194: Make DHCP module more adjustable

Submitter:  David Cermak <david_cermak>
Submitted:  Tue 05 Apr 2022 03:25:43 PM UTC
   
 
Category:  DHCP Priority:  5 - Normal
Status:  None Privacy:  Public
Assigned to:  None Open/Closed:  Open
Planned Release:  None

Jump to the original submission

Fri 12 May 2023 09:08:20 AM UTC, comment #7: 

1) Adding a hook for configuration is not a bad thing. But in this case, I think it might be a more common need to restore an address after rebooting. So I'd rather have that explicitly than "hiding" it in a hook + comment, that's all.

4) Thanks for checking doxygen. I should have seen that myself on the github runner results...

Simon Goldschmidt <goldsimon>
Group administrator
Fri 12 May 2023 08:55:00 AM UTC, comment #6: 

1) Thanks for suggestion. Have reworked the patch to add this `dhcp_start_from_reboot()` API.
(the reason for making it so generic was to minimize requested changes, providing versatility if users wanted to jump to any other state (even violating the spec), my workaround to access the internals was this macro magic: https://github.com/espressif/esp-idf/blob/54576b7528b182256c027de86eb605a172bc2821/components/lwip/port/include/lwipopts.h#L328-L333)

4) Used this MACRO="VALUE" (quoted) notation deliberately in the comments, but I agree that using #define MACRO VALUE is more descriptive.

> took the liberty to slightly adapt the path while applying, but only in the example comment.


This comment broke the doxygen. The attached patch fixes the build by enclosing it within `\code` tags.

2) 3) Thanks for accepting the patches!


(file #54733, file #54734)

David Cermak <david_cermak>
Thu 11 May 2023 07:04:38 PM UTC, comment #5: 

3) I've took the liberty to rename the option and move it to opt.h before applying

4) Again, I've took the liberty to slightly adapt the path while applying, but only in the example comment.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 11 May 2023 06:31:51 PM UTC, comment #4: 

1) Now I get the purpose of that hook. Instead of making it too generic, shouldn't we rather make it explicit and let the hook provide what's required to request the old address? Because the way your example code looks, you're making the application depend on lwIP internals and there's a very high risk that your hook won't work with the next version of lwIP when internals have changed.

You even could add a function 'dhcp_start_from_reboot(netif, old_address)' that could be called instead of 'dhcp_start(netif)' after boot.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 11 May 2023 05:41:25 PM UTC, comment #3: 

4) Renamed to DHCP_ADD_EXTRA_REQUEST_OPTIONS and documented with an example

(file #54726)

David Cermak <david_cermak>
Thu 11 May 2023 05:40:22 PM UTC, comment #2: 

Updated per review:

1) Documented in opts.h with example, renamed to LWIP_HOOK_DHCP_SKIP_DISCOVER_AFTER_START()

2) Documented (directly in the code), added dhcp_timeout_t type which defaults to u16_t, replaced function with macros, split into two commits (0002-Fix1A, 0002-Fix1B) as suggested

3) Added DHCP_DISCOVER_ADD_NETIF_HOSTNAME to enable/disable the hostname option in discover packets

(file #54722, file #54723, file #54724, file #54725)

David Cermak <david_cermak>
Wed 10 May 2023 07:44:59 PM UTC, comment #1: 

Let me respond to each patch:

1) Hooks must be documented in opt.h, see LWIP_HOOK_IP4_ROUTE_SRC. Also, POST_INIT might not be a name that is descriptive enough for what it does (aborting discover after start?)? Please also describe in which situations this is used, describe an example.

2) DHCP_DEFINE_CUSTOM_TIMEOUTS is not described anywhere; u16_t/u32_t change should be a no-op if this feature is not enabled; 'inline' is not portable; also, maybe the changes from in-place-code to defines calculating the timeouts could be in a separate commit from adding the custom tmieouts?

3) This should be configurable. Not everyone wants to the hostname to be added here.

4) This is not a hook. A hook is a function to be called at some point to change he behaviour. I'd just rename the define. But what we need is an example of the contents of this define (i.e., in this case, you need the define to be a comma-separated list starting with a comma; this needs to be clearly documented!)

Simon Goldschmidt <goldsimon>
Group administrator
Tue 05 Apr 2022 03:25:43 PM UTC, original submission:  

Adding 4 separate patches, that should enable users to redefine or adjust various DHCP parameters per specific port needs. (to support some changes in the Espressif's fork: https://github.com/espressif/esp-lwip)

Each patch contains a detailed description in the commit message, a brief overview here:
1) Add-post_init-hook-to-allow-for-skipping-discovery -- to enable devices claim their last bound address after reset
2) Enable-custom-config-for-timeouts-thresholds-backoff -- to adjust timing parameters especially in wireless environment
3) Set-hostname-for-DISCOVER-packets -- fixes potential rejected REQUEST by some servers
4) Add-hook-for-appending-extra-client-s-request-opts -- to support extra options including requested items (VCI/VSI options)

David Cermak <david_cermak>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #54722:  0001-Fix1-dhcp-Add-post_init-hook-to-allow-for-skipping-discov.patch added by david_cermak (3KiB - text/x-patch - Attaching update patches with "Fix1" suffix)
file #54723:  0002-Fix1B-dhcp-Enable-custom-config-for-timeouts-thresholds-ba.patch added by david_cermak (6KiB - text/x-patch - Attaching update patches with "Fix1" suffix)
file #54724:  0002-Fix1A-dhcp-Move-in-place-timeout-threshold-calculations-to.patch added by david_cermak (6KiB - text/x-patch - Attaching update patches with "Fix1" suffix)
file #54725:  0003-Fix1-dhcp-Set-hostname-for-DISCOVER-packets.patch added by david_cermak (2KiB - text/x-patch - Attaching update patches with "Fix1" suffix)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by david_cermak (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-05-12 david_cermak Attached File- Added 0001-Fix2-dhcp-Add-dhcp_start_from_reboot-API-to-start-DHCP-wi.patch, #54733
        Attached File- Added 0004-Fix2-dhcp-docs-Fix-doxygen-build-to-mark-define-in-commen.patch, #54734
    2023-05-11 david_cermak Attached File- Added 0004-Fix1-dhcp-Add-macro-for-appending-extra-client-s-request-.patch, #54726
    2023-05-11 david_cermak Attached File- Added 0001-Fix1-dhcp-Add-post_init-hook-to-allow-for-skipping-discov.patch, #54722
        Attached File- Added 0002-Fix1B-dhcp-Enable-custom-config-for-timeouts-thresholds-ba.patch, #54723
        Attached File- Added 0002-Fix1A-dhcp-Move-in-place-timeout-threshold-calculations-to.patch, #54724
        Attached File- Added 0003-Fix1-dhcp-Set-hostname-for-DISCOVER-packets.patch, #54725
    2022-04-05 david_cermak Attached File- Added 0001-dhcp-Add-post_init-hook-to-allow-for-skipping-discov.patch, #53045
        Attached File- Added 0003-dhcp-Set-hostname-for-DISCOVER-packets.patch, #53046
        Attached File- Added 0002-dhcp-Enable-custom-config-for-timeouts-thresholds-ba.patch, #53047
        Attached File- Added 0004-dhcp-Add-hook-for-appending-extra-client-s-request-o.patch, #53048

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code