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

Add support for hostname request in lwIP interface #15506

Closed
wants to merge 2 commits into from
Closed

Add support for hostname request in lwIP interface #15506

wants to merge 2 commits into from

Conversation

guilhermerc
Copy link

Summary of changes

Add support for hostname request in lwIP interface.

There is a discussion about this functionality here: How to set device network hostname on MBED OS 6?

Impact of changes

Migration actions required

Documentation


Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[X] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[X] Tests / results supplied as part of this PR

Reviewers

@pan-


Like what is done for user MAC address, add methods for setting
hostname. The underlying network stack can then request this to the
local DNS through DHCP.
If hostname is provided, request it to local DNS through DHCP.
@guilhermerc
Copy link
Author

Follows an usage example. It was tested on mbed LPC1768.

#include "EthernetInterface.h"
#include "mbed.h"

int main() {
    EthernetInterface net;
    char hostname[] = "example-hostname";
    nsapi_error_t nsapi_status;

    nsapi_status = net.set_hostname(hostname);
    assert(nsapi_status == NSAPI_ERROR_OK);
    debug("[main] net.get_hostname: %s\n", net.get_hostname());

    do {
        ThisThread::sleep_for(5s);
        nsapi_status = net.connect();
        debug("[main] net.connect rc: %d\n", nsapi_status);
    } while(nsapi_status != NSAPI_ERROR_OK &&
        nsapi_status != NSAPI_ERROR_IS_CONNECTED);

    while(true) {}
}

@guilhermerc guilhermerc marked this pull request as ready for review April 16, 2024 19:13
@@ -441,6 +442,11 @@ nsapi_error_t LWIP::add_ethernet_interface(EMAC &emac, bool default_if, OnboardN
interface->memory_manager = &memory_manager;
interface->ppp_enabled = false;

hostname = user_network_interface->get_hostname();
if(hostname) {
netif_set_hostname(&interface->netif, hostname);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does doing this in add_ethernet_interface() mean that you have to call set_hostname() before adding the Ethernet interface? What about if the user calls set_hostname() later? We should probably document this in the NetworkInterface comments.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, EthernetInterface::set_hostname should be called before EthernetInterface::connect (which calls LWIP::add_ethernet_interface). If it's called later, EthernetInterface::set_hostname will return NSAPI_ERROR_BUSY, as the interface is already initialized.

This behaves exact like EthernetInterface::set_mac_address.

Copy link
Author

@guilhermerc guilhermerc Apr 17, 2024

Choose a reason for hiding this comment

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

We can rewrite NetworkInterface::set_hostname comments to address this behavior:

     *  @retval         NSAPI_ERROR_BUSY if interface is already initialized

I just think we should change it in both NetworkInterface::set_hostname and NetworkInterface::set_mac_address comments for consistency. What do you think?

@guilhermerc
Copy link
Author

I'm closing this PR due to Mbed OS EOL.

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