Set pico_w unique dhcp_hostname or configured hostname #1563

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.

Sign up for GitHub

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

Jump to bottom

Merged

bettio merged 1 commit into atomvm:release-0.6 from UncleGrumpy:rel0.6_pico_hostname

Jun 7, 2025

Collaborator

UncleGrumpy commented Mar 11, 2025

Correctly set dhcp_hostname from sta_config_options() and/or ap_config_options(), or, if undefined, sets a unique hostname to the default device name of atomvm-${DEVICE_MAC}. Formerly all pico_w devices tried to register to an access point with the factory default PicoW hostname, which would make all subsequent pico devices to connect to an access point, after the first one, unreachable by hostname, and cause unpredictable problems for some routers.

Fixes #1094 on release-0.6 branch

IMPORTANT: DO NOT FORWARD this commit to main branch. Breaking changes for rp2 (formerly rp2040) platform necessitate a seaprate PR to fix this issue for release-0.6, a fix has already been submitted to main branch (PR #1173).

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

UncleGrumpy force-pushed the rel0.6_pico_hostname branch from 3799ea5 to 4f00554 Compare

May 9, 2025 22:31

UncleGrumpy changed the title ~~Set pico_w dhcp_hostname~~ Set pico_w unique dhcp_hostname or configured hostname

bettio requested changes

View reviewed changes

src/platforms/rp2040/src/lib/networkdriver.c Outdated

+                  // parameter is ignored and both interfaces have the same MAC address.
+                  int err = cyw43_wifi_get_mac(&cyw43_state, CYW43_ITF_STA, mac);
+                  if (err) {
+                      return globalcontext_make_atom(global, ATOM_STR("\x10", "device_mac_error"));

Collaborator

bettio Jun 3, 2025

In general, shouldn't error be a tuple, such as {error, Reason}?
Anyway in this specific case I'd let the caller handle the error.

src/platforms/rp2040/src/lib/networkdriver.c Outdated

@@ @@ -211,10 +215,29 @@ static void send_sntp_sync(struct timeval *tv) @@
                   END_WITH_STACK_HEAP(heap, driver_data->global);
               }
+              // param should be pointer to malloc'd destination to copy device hostname
+              // return ok atom or error as atom
+              static term get_default_device_name(char *name, GlobalContext *global)

Collaborator

bettio Jun 3, 2025

We might rework this function to be just a helper for writing to the hostname buffer.
Such as static bool write_default_device_name(char *out, size_t out_len);
The caller may decide how to handle the error.

src/platforms/rp2040/src/lib/networkdriver.c Outdated

+                  if (err) {
+                      return globalcontext_make_atom(global, ATOM_STR("\x10", "device_mac_error"));
+                  }
+                  size_t buf_size = DEFAULT_HOSTNAME_SIZE;

Collaborator

bettio Jun 3, 2025

if we take this as parameter, we might make this code more solid and less error prone.

src/platforms/rp2040/src/lib/networkdriver.c Outdated

+                          cyw43_arch_disable_sta_mode();
+                          return OUT_OF_MEMORY_ATOM;
+                      }
+                      memcpy(driver_data->hostname, buf, buf_size);

Collaborator

bettio Jun 3, 2025

strlen + malloc + memcpy = strdup ;)
In general: let's just use strdup!
In this specific case I didn't understand why we don't just keep the buffer return from interop_term_to_string.

src/platforms/rp2040/src/lib/networkdriver.c Outdated

                       }
                   }
+                  if (term_is_invalid_term(hostname_term)) {

Collaborator

bettio Jun 3, 2025

This code feels like a copy & paste of another block, can we factor out this?

Collaborator Author

UncleGrumpy Jun 6, 2025

Indeed. This has been refactored.

UncleGrumpy force-pushed the rel0.6_pico_hostname branch 4 times, most recently from af39282 to 632bcde Compare

June 6, 2025 08:50

bettio requested changes

View reviewed changes

src/platforms/rp2040/src/lib/networkdriver.c Show resolved Hide resolved

src/platforms/rp2040/src/lib/networkdriver.c Outdated

+                  char *buf = malloc(size);
+                  snprintf(buf, size,
+                      "atomvm-%02x%02x%02x%02x%02x%02x", mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
+                  *out = strndup(buf, size);

Collaborator

bettio Jun 6, 2025

I think I didn't explain well what I meant with my previous commit.
strdup is good when for any reason we need to duplicate a string, and it is equivalent of strlen + malloc + memcpy.
Here instead there is no reason for this: we already allocating some new memory, that we can use in any manner we like.
So just do: *out = buf;

Collaborator Author

UncleGrumpy Jun 6, 2025

With the changes now out has not been allocated yet, is it better to malloc and check for NULL before *out = buf or just use strdup? We can't keep buf because there is no way to free it later if the interface or driver is stopped.

src/platforms/rp2040/src/lib/networkdriver.c Show resolved Hide resolved

src/platforms/rp2040/src/lib/networkdriver.c Outdated

+                  }
+                  char *buf = malloc(size);
+                  snprintf(buf, size,
+                      "atomvm-%02x%02x%02x%02x%02x%02x", mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);

Collaborator

bettio Jun 6, 2025

Let's use DEFAULT_HOSTNAME_FMT here snprintf(buf, size, DEFAULT_HOSTNAME_FMT...

src/platforms/rp2040/src/lib/networkdriver.c Outdated

+                          cyw43_arch_disable_sta_mode();
+                          return DriverOOM;
+                      }
+                      *out = strdup(buf);

Collaborator

bettio Jun 6, 2025

also here, there is no need to duplicate buf, just *out = buf.

src/platforms/rp2040/src/lib/networkdriver.c Outdated

+                  return DriverOK;
+              }
+              static int set_interface_dhcp_name(char **out, term dhcp_name)

Collaborator

bettio Jun 6, 2025

I usually suggest the convention to leave the out arg after all input parameters.
Also return type here should be likely: enum DriverErrorCodeType instead of int.
static enum DriverErrorCodeType set_interface_dhcp_name(term dhcp_name, char **out)

src/platforms/rp2040/src/lib/networkdriver.c Outdated

@@ @@ -211,15 +244,61 @@ static void send_sntp_sync(struct timeval *tv) @@
                   END_WITH_STACK_HEAP(heap, driver_data->global);
               }
+              static int write_default_device_name(char **out, size_t size)

Collaborator

bettio Jun 6, 2025

Type here should be likely: enum DriverErrorCodeType instead of int.
So: static enum DriverErrorCodeType write_default_device_name(char **out, size_t size)

Collaborator Author

UncleGrumpy Jun 6, 2025

Thanks, I forgot to update that after adding the enum.

CHANGELOG.md Outdated

@@ @@ -67,6 +67,7 @@ memory error @@
               - Fixed nif_atomvm_posix_read GC bug
               - Fixed `erlang:is_number/1` function, now returns true also for floats
               - Fixed unlink protocol and add support for `link/1` on ports
+              - Correctly set Pico-W unique hostname, use `dhcp_hostname` if present in config.

Collaborator

bettio Jun 6, 2025 •

edited

Loading

I think we should at least add also an entry to "Added" in order to mention that now we allow to configure the hostname (using a certain option).
While under "Fixed" I would just focus on what has been fixed, also I don't find this entry really clear.
Can you clearly state what was not working and what we fixed?

UncleGrumpy force-pushed the rel0.6_pico_hostname branch from 632bcde to 8341a72 Compare

June 6, 2025 17:55


          Set inique dhcp_hostname, or configured name on pico_w

e76ab09

Correctly set `dhcp_hostname` from `sta_config_options()` and/or `ap_config_options()`, or, if
undefined, sets a unique hostname to the default device name of `atomvm-${DEVICE_MAC}`. Formerly
all pico_w devices tried to register to an access point with the factory default `PicoW` hostname,
which would make all subsequent pico devices to connect to an access point, after the first one,
unreachable by hostname, and cause unpredictable problems for some routers.

Fixes atomvm#1094 on release-0.6 branch

IMPORTANT: DO NOT FORWARD this commit to main branch. Breaking changes for rp2 (formerly rp2040)
platform necessitate a seaprate PR to fix this issue for release-0.6, a fix has already been
submitted to main branch (PR atomvm#1173).

Signed-off-by: Winford <[email protected]>

UncleGrumpy force-pushed the rel0.6_pico_hostname branch from 8341a72 to e76ab09 Compare

June 6, 2025 18:02

bettio approved these changes

View reviewed changes

bettio mentioned this pull request

Set unique dhcp_hostname, or configured hostname on pico_w #1173

Closed

bettio merged commit b01852e into atomvm:release-0.6

8 checks passed

bettio added a commit that referenced this pull request


          Forward port changes from v0.6 release branch

ec2dba5

Merge mostly chores from release-0.6 branch, but also PR for setting pico_w
unique dhcp_hostname or configured hostname (#1563).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet