Create hotspot #5577
Create hotspot #5577
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
home-assistant
bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
📝 WalkthroughWalkthroughThe pull request introduces enhanced Wi-Fi configuration capabilities across multiple files in the Supervisor project. The changes primarily focus on adding support for specifying Wi-Fi band and channel settings, expanding the existing network configuration infrastructure. New constants, attributes, and enumeration values are introduced to facilitate more granular wireless network configuration, including the addition of a Changes
Sequence DiagramsequenceDiagram
participant API as Network API
participant Configuration as Network Configuration
participant DBus as DBus Interface
participant Host as Host Configuration
API->>Configuration: Request Wi-Fi Configuration
Configuration->>DBus: Retrieve Wireless Settings
DBus-->>Configuration: Return Wireless Properties
Configuration->>Host: Map Network Interface
Host->>Host: Apply Band and Channel Settings
Host-->>Configuration: Return Updated Configuration
Configuration-->>API: Provide Detailed Wi-Fi Config
The sequence diagram illustrates the flow of Wi-Fi configuration data through different components, highlighting how the new band and channel attributes are processed and propagated across the system. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
supervisor/dbus/network/setting/generate.py (1)
61-69: Simplify the IPv4 method comparisonThe IPv4 method comparison can be simplified using a set for better readability.
- elif ( - ipv4setting.method == InterfaceMethod.STATIC - or ipv4setting.method == InterfaceMethod.SHARED - ): + elif ipv4setting.method in {InterfaceMethod.STATIC, InterfaceMethod.SHARED}:🧰 Tools
🪛 Ruff (0.8.2)
62-63: Consider merging multiple comparisons. Use a
setif the elements are hashable.Merge multiple comparisons
(PLR1714)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
supervisor/api/network.py(5 hunks)supervisor/const.py(1 hunks)supervisor/dbus/const.py(1 hunks)supervisor/dbus/network/configuration.py(1 hunks)supervisor/dbus/network/setting/__init__.py(2 hunks)supervisor/dbus/network/setting/generate.py(3 hunks)supervisor/host/configuration.py(5 hunks)supervisor/host/const.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
supervisor/const.py (6)
Pattern */**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern */**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern */**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern */**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern */**(html|markdown|md): do not comment on HTML used for icons
Pattern */**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
🪛 Ruff (0.8.2)
supervisor/dbus/network/setting/generate.py
62-63: Consider merging multiple comparisons. Use a set if the elements are hashable.
Merge multiple comparisons
(PLR1714)
🔇 Additional comments (16)
supervisor/host/const.py (2)
16-16: LGTM: Interface method additionThe SHARED method is a valid addition for NetworkManager interface configuration, typically used for creating hotspots.
35-40: LGTM: WiFi band enumerationThe WifiBand enum correctly represents the standard WiFi frequency bands (5GHz and 2.4GHz) with appropriate string values.
supervisor/dbus/network/configuration.py (1)
36-37: LGTM: Wireless properties extensionThe new band and channel attributes are correctly typed and align with NetworkManager's D-Bus API requirements.
supervisor/host/configuration.py (4)
58-59: LGTM: WiFi configuration attributesThe band and channel attributes are correctly added to the WifiConfig dataclass with appropriate type hints.
196-196: LGTM: Interface method mappingThe SHARED method is correctly mapped between NetworkManager and internal representations.
266-266: LGTM: Fixed VLAN mapping return typeThe return type annotation for _map_nm_vlan has been corrected to VlanConfig | None.
243-248: Verify AP mode configurationThe band and channel are only set in AP mode, which is correct for hotspot functionality. However, ensure that the band selection is validated against the device's capabilities.
✅ Verification successful
AP mode WiFi band configuration is correctly implemented
The code correctly handles band selection in AP mode. The WifiBand enum ensures only valid values ("a" for 5GHz or "bg" for 2.4GHz) can be set. While there's no explicit device capability validation in this layer, this is typically handled by the wireless driver/hardware layer which will reject unsupported configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the code handles device capabilities for WiFi bands ast-grep --pattern 'if $_device.capabilities: $$$ band = $$$ $$$'Length of output: 73
Script:
#!/bin/bash # Find WifiBand definition and usage ast-grep --pattern 'class WifiBand' # Search for device capabilities in WiFi context rg -A 5 "capabilities.*wifi" -i # Search for band validation patterns rg -A 5 "band.*valid|valid.*band" -i # Find where band is used with device rg -A 5 "device.*band|band.*device" -iLength of output: 92272
Script:
#!/bin/bash # Find WifiBand definition rg -A 5 "class WifiBand|enum WifiBand" -i # Look for WiFi settings validation rg -A 5 "def.*validate.*wifi|wifi.*validate" -i # Search for band-related code rg -A 5 "band.*settings|settings.*band" -iLength of output: 119217
supervisor/dbus/network/setting/generate.py (1)
211-211: LGTM: Enhanced wireless configurationThe wireless configuration correctly handles the mode, band, and channel settings with appropriate D-Bus variants.
Also applies to: 218-223
supervisor/dbus/network/setting/__init__.py (2)
51-52: LGTM! Constants follow naming convention.The new constants for wireless band and channel follow the existing naming pattern and are appropriately placed with other wireless-related constants.
239-240: LGTM! Wireless properties properly extended.The reload method correctly retrieves the new band and channel attributes from the wireless configuration data.
supervisor/api/network.py (4)
15-16: LGTM! Attributes properly imported.New attributes for band and channel are correctly imported and placed with related network attributes.
84-85: LGTM! Schema validation properly configured.The schema correctly defines band and channel as optional fields with appropriate type coercion:
- WifiBand for band
- int for channel
119-120: LGTM! Struct properly extended.The wifi_struct function correctly includes the new band and channel fields in the returned dictionary.
236-237: LGTM! Interface update properly handles new fields.The interface_update method correctly processes the new band and channel attributes when updating WiFi configuration.
supervisor/dbus/const.py (1)
207-207: LGTM! Interface method properly extended.The SHARED interface method is correctly added to support hotspot functionality.
supervisor/const.py (1)
122-122: LGTM! Constant properly defined.The ATTR_BAND constant is correctly defined and follows the existing naming convention.
|
Hi, thank you for your PR! However, I do wonder if being a hotspot should really be in scope for Supervisor. What is your use case here? I fear that this will open up a huge can of worms. Most WiFi chipsets which are focused for clients have limitations when running as hotspot. Also, running a hotspot requires solid regulatory domain configuration to avoid violating local RF rules. Afaik, being a station that is not that problematic as the WiFi chipset learns from the AP it is connecting to what regulatory domain should be used. But as an AP, it would be "on us". |
|
i want to create a private WiFi network for my cameras, which support only 2.4GHz, to reclaim the bandwidth on my home network. NetworkManager can do this easily, i've done it this way a bunch of times and tried it my current hardware, but supervisor subsequently corrupts the configuration and destroys the network. as i investigated i realized with a relatively small change, a couple settings and minor fixes, supervisor will play nice with NetworkManager and H-A gains a capability a few folks really want. as far as the regulatory stuff goes: supervisor is barely involved, it just hands off to NetworkManager, which is fully aware of these concerns and will refuse illegal settings, including when the chipset cannot support AP. |
- add IP method "shared" - new WiFi parameters "band" and "channel" NetworkManager provides the magic
jkt628
force-pushed
the
feat/ipv4-method-shared
branch
from
a02d537 to
1ef04a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
supervisor/dbus/network/setting/generate.py (1)
61-69: Consider optimizing the method comparison.The implementation correctly supports both static and shared methods, but could be more concise using a set comparison.
Consider this optimization:
- elif ( - ipv4setting.method == InterfaceMethod.STATIC - or ipv4setting.method == InterfaceMethod.SHARED - ): + elif ipv4setting.method in {InterfaceMethod.STATIC, InterfaceMethod.SHARED}: ipv4[CONF_ATTR_IPV4_METHOD] = ( Variant("s", "manual") if ipv4setting.method == InterfaceMethod.STATIC else Variant("s", "shared") )🧰 Tools
🪛 Ruff (0.8.2)
62-63: Consider merging multiple comparisons. Use a
setif the elements are hashable.Merge multiple comparisons
(PLR1714)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
supervisor/api/network.py(5 hunks)supervisor/const.py(1 hunks)supervisor/dbus/const.py(1 hunks)supervisor/dbus/network/configuration.py(1 hunks)supervisor/dbus/network/setting/__init__.py(2 hunks)supervisor/dbus/network/setting/generate.py(3 hunks)supervisor/host/configuration.py(5 hunks)supervisor/host/const.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- supervisor/const.py
- supervisor/dbus/network/configuration.py
- supervisor/host/const.py
- supervisor/dbus/network/setting/init.py
- supervisor/dbus/const.py
🧰 Additional context used
🪛 Ruff (0.8.2)
supervisor/dbus/network/setting/generate.py
62-63: Consider merging multiple comparisons. Use a set if the elements are hashable.
Merge multiple comparisons
(PLR1714)
🔇 Additional comments (7)
supervisor/host/configuration.py (3)
15-15: LGTM!The import is correctly placed with other related network constants.
196-196: LGTM! The SHARED method is essential for hotspot functionality.The addition of the SHARED method to the mapping is correct and necessary for enabling IP sharing when operating as a hotspot.
58-59: LGTM! Consider adding value validation for channel.The optional band and channel fields are correctly typed. However, consider adding validation for the channel number since valid channels depend on the selected band and regulatory domain.
Let's check if there's any validation for channel numbers in the codebase:
supervisor/api/network.py (1)
15-16: LGTM!The new attribute constants are correctly placed with other related network attributes.
supervisor/dbus/network/setting/generate.py (3)
20-21: LGTM! Import changes are consistent with the codebase.The new wireless band and channel constants follow the existing naming conventions and are properly grouped with related wireless configuration imports.
211-211: Good improvement: Using dynamic mode instead of hardcoded value.The change from hardcoded "infrastructure" to
interface.wifi.modeprovides better flexibility for different WiFi modes, including hotspot functionality.
218-223: Verify regulatory compliance for band and channel settings.While NetworkManager handles regulatory compliance, consider adding validation or documentation about:
- Supported bands and their corresponding channel ranges
- How NetworkManager handles invalid configurations
- Impact of regulatory domain settings on available options
This will help users understand the limitations and avoid configuration issues.
Let's verify the NetworkManager documentation and behavior:
✅ Verification successful
NetworkManager handles regulatory compliance appropriately
The code correctly delegates wireless regulatory compliance to NetworkManager, which enforces country-specific restrictions for bands and channels. Adding validation at the supervisor level would be redundant and potentially problematic as regulatory rules vary by country and can change over time.
If you need to understand the regulatory limitations:
- Check your system's regulatory domain:
iw reg get- Consult NetworkManager documentation for your specific country
- Use
nmclito view available bands and channels🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check NetworkManager documentation for wireless regulatory info # Search for regulatory domain handling in NetworkManager docs rg -i "regulatory|domain|country" --type md # Look for existing validation patterns in the codebase rg -p "band|channel.*valid" --type pythonLength of output: 120
Script:
#!/bin/bash # Search for wireless configuration validation and documentation # Look in Python files for band/channel validation rg -p "band|channel.*valid" "*.py" # Search for wireless configuration related constants and enums rg -p "class.*(Wifi|Wireless)" "*.py" -A 10 # Search for test files related to wireless settings fd -e py -e md -e rst | grep -i "wifi\|wireless\|test" # Look for any documentation about wireless configuration fd "README" -e md -e rst | xargs catLength of output: 11109
Script:
#!/bin/bash # Examine wireless implementation and tests cat supervisor/dbus/network/wireless.py cat tests/dbus/network/test_wireless.py cat tests/dbus/network/setting/test_generate.py cat tests/dbus_service_mocks/network_device_wireless.pyLength of output: 13087
| # Band and Channel | ||
| band = channel = None | ||
| if mode == WifiMode.AP: | ||
| band = WifiBand(inet.settings.wireless.band) | ||
| channel = inet.settings.wireless.channel | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add regulatory domain and hardware capability checks.
The current implementation doesn't validate if the selected band and channel are:
- Supported by the WiFi hardware
- Allowed in the current regulatory domain
- Valid for the selected band (e.g., channel 36 is invalid for 2.4GHz)
Consider adding validation by leveraging NetworkManager's capabilities:
# Band and Channel
band = channel = None
if mode == WifiMode.AP:
+ # Get regulatory domain and hardware capabilities
+ if inet.wireless and inet.wireless.capabilities:
+ # Validate band is supported
+ if inet.settings.wireless.band in inet.wireless.capabilities.bands:
+ band = WifiBand(inet.settings.wireless.band)
+ # Validate channel is allowed
+ if (
+ inet.settings.wireless.channel
+ and inet.settings.wireless.channel
+ in inet.wireless.capabilities.channels[band]
+ ):
+ channel = inet.settings.wireless.channel
- band = WifiBand(inet.settings.wireless.band)
- channel = inet.settings.wireless.channel📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Band and Channel | |
| band = channel = None | |
| if mode == WifiMode.AP: | |
| band = WifiBand(inet.settings.wireless.band) | |
| channel = inet.settings.wireless.channel | |
| # Band and Channel | |
| band = channel = None | |
| if mode == WifiMode.AP: | |
| # Get regulatory domain and hardware capabilities | |
| if inet.wireless and inet.wireless.capabilities: | |
| # Validate band is supported | |
| if inet.settings.wireless.band in inet.wireless.capabilities.bands: | |
| band = WifiBand(inet.settings.wireless.band) | |
| # Validate channel is allowed | |
| if ( | |
| inet.settings.wireless.channel | |
| and inet.settings.wireless.channel | |
| in inet.wireless.capabilities.channels[band] | |
| ): | |
| channel = inet.settings.wireless.channel | |
supervisor/api/network.py
Outdated
| config.get(ATTR_BAND, None), | ||
| config.get(ATTR_CHANNEL, None), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for invalid band/channel combinations.
The current implementation doesn't handle potential errors when:
- The specified band is not supported by the hardware
- The channel is invalid for the selected band
- The band/channel combination violates regulatory domain restrictions
Consider adding error handling:
elif key == ATTR_WIFI:
+ # Validate band and channel combination
+ band = config.get(ATTR_BAND)
+ channel = config.get(ATTR_CHANNEL)
+ if band and channel:
+ # Get interface capabilities
+ if not interface.wireless or not interface.wireless.capabilities:
+ raise APIError("Unable to get wireless capabilities")
+
+ # Validate band is supported
+ if band not in interface.wireless.capabilities.bands:
+ raise APIError(f"Band {band} is not supported by the hardware")
+
+ # Validate channel is allowed
+ if channel not in interface.wireless.capabilities.channels[band]:
+ raise APIError(
+ f"Channel {channel} is not valid for band {band} "
+ "or not allowed in your region"
+ )
+
interface.wifi = WifiConfig(
config.get(ATTR_MODE, WifiMode.INFRASTRUCTURE),
config.get(ATTR_SSID, ""),
config.get(ATTR_AUTH, AuthMethod.OPEN),
config.get(ATTR_PSK, None),
None,
- config.get(ATTR_BAND, None),
- config.get(ATTR_CHANNEL, None),
+ band,
+ channel,
)Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
supervisor should leave validation to NetworkManager, which understands regulatory concerns.
There was a problem hiding this comment.
@jkt628 You're absolutely right. NetworkManager is better equipped to handle the validation of wireless settings and regulatory compliance. I'll retract my suggestion.
[learning]
- Don't suggest adding validation for wireless settings (band, channel, etc.) in the Supervisor as these should be delegated to NetworkManager which has built-in support for hardware capabilities and regulatory compliance.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
supervisor/api/network.py
Outdated
| vol.Optional(ATTR_BAND): vol.Coerce(WifiBand), | ||
| vol.Optional(ATTR_CHANNEL): vol.Coerce(int), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for channel numbers.
While the schema correctly defines the types, it lacks validation for channel numbers. Consider adding:
- Range validation for channel numbers (e.g., 1-14 for 2.4GHz, 36-165 for 5GHz)
- Conditional validation where channel must be provided if band is specified
vol.Optional(ATTR_SSID): str,
vol.Optional(ATTR_PSK): str,
- vol.Optional(ATTR_BAND): vol.Coerce(WifiBand),
- vol.Optional(ATTR_CHANNEL): vol.Coerce(int),
+ vol.Optional(ATTR_BAND): vol.Coerce(WifiBand),
+ vol.Optional(ATTR_CHANNEL): vol.All(
+ vol.Coerce(int),
+ vol.Range(min=1, max=165),
+ msg="Channel must be between 1 and 165"
+ ),Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@jkt628 You make an excellent point. Delegating the validation to NetworkManager is indeed the better approach since it has comprehensive knowledge of regulatory domains and hardware capabilities. This also aligns well with the separation of concerns principle. I'll withdraw my suggestion about adding channel validation.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Fetch the comment content
gh api -X GET /repos/home-assistant/supervisor/pulls/comments/1930610581
Length of output: 2748
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
Hm, yeah I am more worried about these soft-limitations, like the 8 client limitations on some (all?) Intel cards. A WiFi/stack designed for stations is not necessarily ready to be an full fledged AP. It is often the case, that from works on my desk to actual production use is more painful then initially anticipated. I am almost certain this will lead to issues in certain scenarios. That said, I also see the technical appeal to it: It can be useful for small setups or to have some few WiFi things separated. Unfortunately, some Home Assistant users think just because the feature is there, it MUST work on their hardware, in all crazy scenarios, reliable and full fledged like any AP, and the Home Assistant team is "obligated" to fix if it's not working for them. So, if we add support for AP mode, we should make clear that AP support is more of a "best-effort" feature: If it works for you, nice! If you have a patch to make your WiFi card work better, sure, PR welcome! But don't expect this to be guaranteed to work in all scenarios under all circumstances. |
NetworkManager provides the magic
Proposed change
create a hotspot using NetworkManager
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates