-
-
Notifications
You must be signed in to change notification settings - Fork 186
Add implementation for NativeSetDeviceName #3243
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds a native Wi‑Fi adapter method to set the device hostname: header declaration, registration in the platform‑neutral native method table with an updated assembly CRC and an extra NULL slot, an ESP32 implementation that sets the STA netif hostname, and AzureRTOS stubs (NOTIMPL/no‑op). Changes
Sequence Diagram(s)sequenceDiagram
participant Managed as Managed (C#)
participant CLR as nanoCLR native entry
participant ESP as esp_netif / ESP-IDF
Managed->>CLR: Invoke NativeSetDeviceName(string hostname)
CLR->>CLR: Read & validate hostname
CLR->>CLR: Validate network interface index (if applicable)
CLR->>ESP: esp_netif_get_handle_from_ifkey("WIFI_STA_DEF")
ESP-->>CLR: netif handle or NULL
alt netif found
CLR->>ESP: esp_netif_set_hostname(netif, hostname)
ESP-->>CLR: esp_err_t (ESP_OK / error)
CLR-->>Managed: return or mapped CLR error
else netif not found
CLR-->>Managed: return CLR_E_INVALID_OPERATION (or mapped error)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (7)📓 Common learnings📚 Learning: 2025-12-02T09:32:22.287ZApplied to files:
📚 Learning: 2024-09-25T11:28:38.536ZApplied to files:
📚 Learning: 2024-10-08T15:52:09.445ZApplied to files:
📚 Learning: 2025-01-22T03:38:57.394ZApplied to files:
📚 Learning: 2024-10-08T15:52:09.445ZApplied to files:
📚 Learning: 2025-01-22T03:38:57.394ZApplied to files:
🧬 Code graph analysis (2)src/System.Device.Wifi/sys_dev_wifi_native.cpp (2)
targets/ESP32/_nanoCLR/System.Device.Wifi/sys_dev_wifi_native_System_Device_Wifi_WifiAdapter.cpp (1)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/System.Device.Wifi/sys_dev_wifi_native.cpp(2 hunks)src/System.Device.Wifi/sys_dev_wifi_native.h(1 hunks)targets/ESP32/_nanoCLR/System.Device.Wifi/sys_dev_wifi_native_System_Device_Wifi_WifiAdapter.cpp(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-10-12T19:00:39.000Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-10-12T19:00:39.000Z
Learning: When working with `nanoCLR_GetNativeAssemblyInformation`, fixed-size assembly names are required, so code that deals with variable-length names cannot be used.
Applied to files:
src/System.Device.Wifi/sys_dev_wifi_native.hsrc/System.Device.Wifi/sys_dev_wifi_native.cpp
📚 Learning: 2024-10-08T15:52:09.445Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-10-08T15:52:09.445Z
Learning: In `nanoCLR_GetNativeAssemblyInformation`, there is no need to return the number of bytes written, as the memory buffer is zeroed, making the string buffer terminated.
Applied to files:
src/System.Device.Wifi/sys_dev_wifi_native.h
🔇 Additional comments (3)
src/System.Device.Wifi/sys_dev_wifi_native.h (1)
69-69: LGTM! Method declaration follows the correct pattern.The native method declaration is properly formatted and consistently placed with other WifiAdapter methods.
src/System.Device.Wifi/sys_dev_wifi_native.cpp (1)
36-43: LGTM! Method lookup table correctly updated.The new method has been properly wired into the method lookup table. The CRC update at line 86 correctly reflects the assembly changes.
targets/ESP32/_nanoCLR/System.Device.Wifi/sys_dev_wifi_native_System_Device_Wifi_WifiAdapter.cpp (1)
11-11: LGTM! Required include for ESP-NETIF APIs.The
esp_netif.hinclude is necessary for the hostname configuration functionality.
...ets/ESP32/_nanoCLR/System.Device.Wifi/sys_dev_wifi_native_System_Device_Wifi_WifiAdapter.cpp
Outdated
Show resolved
Hide resolved
...ets/ESP32/_nanoCLR/System.Device.Wifi/sys_dev_wifi_native_System_Device_Wifi_WifiAdapter.cpp
Outdated
Show resolved
Hide resolved
|
@zandiarash please also use the proper PR template. |
|
Also, please adjust all the native devices. It's find to throw an exception but the build of other devices than ESP32 will break. |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
c26eae9 to
b1deeab
Compare
Thank you for the guidance. |
...reRTOS/ST/_nanoCLR/System.Device.Wifi/sys_dev_wifi_native_System_Device_Wifi_WifiAdapter.cpp
Show resolved
Hide resolved
...reRTOS/ST/_nanoCLR/System.Device.Wifi/sys_dev_wifi_native_System_Device_Wifi_WifiAdapter.cpp
Outdated
Show resolved
Hide resolved
...ets/ESP32/_nanoCLR/System.Device.Wifi/sys_dev_wifi_native_System_Device_Wifi_WifiAdapter.cpp
Outdated
Show resolved
Hide resolved
...reRTOS/ST/_nanoCLR/System.Device.Wifi/sys_dev_wifi_native_System_Device_Wifi_WifiAdapter.cpp
Show resolved
Hide resolved
...ets/ESP32/_nanoCLR/System.Device.Wifi/sys_dev_wifi_native_System_Device_Wifi_WifiAdapter.cpp
Outdated
Show resolved
Hide resolved
...ets/ESP32/_nanoCLR/System.Device.Wifi/sys_dev_wifi_native_System_Device_Wifi_WifiAdapter.cpp
Outdated
Show resolved
Hide resolved
...ets/ESP32/_nanoCLR/System.Device.Wifi/sys_dev_wifi_native_System_Device_Wifi_WifiAdapter.cpp
Show resolved
Hide resolved
...ets/ESP32/_nanoCLR/System.Device.Wifi/sys_dev_wifi_native_System_Device_Wifi_WifiAdapter.cpp
Outdated
Show resolved
Hide resolved
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
If you can do it, that's preferable. |
@josesimoes I’ve already made everything into a single commit (your review messages and bot suggestions).👍 |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
...reRTOS/ST/_nanoCLR/System.Device.Wifi/sys_dev_wifi_native_System_Device_Wifi_WifiAdapter.cpp
Outdated
Show resolved
Hide resolved
josesimoes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Description
NativeSetDeviceName().Motivation and Context
I should mention that I am not familiar with C++ and after I changed nf-interpreter project I flashed my chip like this
nanoff --platform esp32 --target ESP32_REV3 --serialport COM6 --update --clrfile "D:\Sources Electronic\Nanoframework - Copy\nf-interpreter\build\nanoCLR.bin"How Has This Been Tested?
Screenshots
Before :


After :
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Platform Notes
✏️ Tip: You can customize this high-level summary in your review settings.