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. |
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
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. |
There was a problem hiding this comment.
Other then the nitpick the code generally looks fine. I also echo @agners concerns on some of the challenges this is likely to pose in production once actually deployed across the myriad of supported systems and hardware people run Supervisor on.
This needs test cases to be merged though. If its true that Network Manager gracefully refuses illegal settings I'd like to see a test case for that. Obviously we're just mocking Network Manager in our tests here but can run a test locally trying to set something that shouldn't work, see how it is handled by Network Manager and mimic that behavior in the appropriate dbus service mock for a test.
sure, i'll add some soonest. |
|
i just figured out how to run pytest and it looks like i broke update test anyway. |
jkt628
force-pushed
the
feat/ipv4-method-shared
branch
from
d6471fb to
0145acd
Compare
jkt628
force-pushed
the
feat/ipv4-method-shared
branch
from
0145acd to
75d41ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
supervisor/host/configuration.py (1)
243-248:⚠️ Potential issueAdd validation for band and channel settings.
The current implementation directly assigns band and channel values without validation. This was previously raised in a past review comment.
Please refer to the previous review comment about adding regulatory domain and hardware capability checks. The implementation should validate:
- If the selected band is supported by the WiFi hardware
- If the channel is allowed in the current regulatory domain
- If the channel is valid for the selected band
🧹 Nitpick comments (2)
tests/api/test_network.py (1)
279-302: Add more test cases for WiFi channel validation.The current test only covers one invalid channel scenario. Consider adding more test cases:
- Valid channels for 2.4GHz band (1-14)
- Valid channels for 5GHz band (36, 40, etc.)
- Invalid channels for both bands
- Channel validation when band is not specified
Add more test cases:
@pytest.mark.parametrize( ("band", "channel", "expected_error"), [ ("bg", 1, None), # Valid 2.4GHz channel ("bg", 14, None), # Valid 2.4GHz channel ("bg", 15, "802-11-wireless.channel: '15' is not a valid channel"), # Invalid 2.4GHz ("a", 36, None), # Valid 5GHz channel ("a", 165, None), # Valid 5GHz channel ("a", 166, "802-11-wireless.channel: '166' is not a valid channel"), # Invalid 5GHz ("a", 1, "802-11-wireless.channel: '1' is not a valid channel"), # Invalid band/channel combo ], ) async def test_api_network_interface_update_wifi_channel_validation( api_client: TestClient, band: str, channel: int, expected_error: str | None ): """Test network interface WiFi API channel validation.""" resp = await api_client.post( f"/network/interface/{TEST_INTERFACE_WLAN_NAME}/update", json={ "enabled": True, "ipv4": {"method": "shared", "address": ["10.42.0.1/24"]}, "ipv6": {"method": "auto"}, "wifi": {"mode": "ap", "ssid": "HotSpot", "band": band, "channel": channel}, }, ) result = await resp.json() if expected_error: assert result["result"] == "error" assert result["message"] == f"Can't create config and activate wlan0: {expected_error}" else: assert result["result"] == "ok"supervisor/dbus/network/setting/generate.py (1)
222-227: Consider validating band and channel before adding to wireless settings.While the conditional addition of band and channel settings is good, consider adding validation before setting these values to ensure they are valid for the hardware and regulatory domain.
if interface.wifi and interface.wifi.band: + # Validate band is supported by hardware + if interface.wifi.band in interface.wireless.capabilities.bands: wireless[CONF_ATTR_802_WIRELESS_BAND] = Variant("s", interface.wifi.band) if interface.wifi and interface.wifi.channel: + # Validate channel is allowed for band and regulatory domain + if (interface.wifi.channel in + interface.wireless.capabilities.channels[interface.wifi.band]): wireless[CONF_ATTR_802_WIRELESS_CHANNEL] = Variant( "u", interface.wifi.channel )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
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(4 hunks)supervisor/host/configuration.py(5 hunks)supervisor/host/const.py(2 hunks)tests/api/test_network.py(1 hunks)tests/dbus_service_mocks/network_manager.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- supervisor/dbus/network/configuration.py
- supervisor/const.py
- supervisor/dbus/network/setting/init.py
- supervisor/dbus/const.py
- supervisor/api/network.py
- supervisor/host/const.py
🔇 Additional comments (1)
supervisor/dbus/network/setting/generate.py (1)
55-66: Good refactoring of address data generation.The extraction of address data generation into a separate function improves code reusability and readability.
jkt628
force-pushed
the
feat/ipv4-method-shared
branch
from
6d0dcd2 to
2b17e26
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
supervisor/host/configuration.py (1)
243-248:⚠️ Potential issueAdd validation for band and channel settings.
The current implementation assigns band and channel values directly without validation. This could lead to issues if:
- The WiFi hardware doesn't support the selected band
- The channel is invalid for the selected band
- The settings violate local regulatory domain rules
Based on the PR objectives and past review comments, this is a critical concern that was previously raised. The implementation should leverage NetworkManager's capabilities to validate these settings.
Also applies to: 261-262
🧹 Nitpick comments (1)
supervisor/dbus/network/setting/generate.py (1)
210-227: Consider adding error handling for invalid band/channel combinations.While the code correctly adds band and channel settings to the wireless configuration, it might be beneficial to add validation to ensure:
- The band and channel combination is valid
- The settings comply with regulatory requirements
if interface.wifi and interface.wifi.band: + # Validate band is supported by the hardware + if interface.wifi.band not in ("a", "bg"): + raise ValueError(f"Invalid band: {interface.wifi.band}") wireless[CONF_ATTR_802_WIRELESS_BAND] = Variant("s", interface.wifi.band) if interface.wifi and interface.wifi.channel: + # Validate channel is valid for the band + valid_channels = { + "a": range(36, 165), # 5GHz channels + "bg": range(1, 14) # 2.4GHz channels + } + band = interface.wifi.band or "bg" # Default to 2.4GHz + if interface.wifi.channel not in valid_channels[band]: + raise ValueError( + f"Invalid channel {interface.wifi.channel} for band {band}" + ) wireless[CONF_ATTR_802_WIRELESS_CHANNEL] = Variant( "u", interface.wifi.channel )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
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(4 hunks)supervisor/host/configuration.py(5 hunks)supervisor/host/const.py(2 hunks)tests/api/test_network.py(1 hunks)tests/dbus_service_mocks/network_manager.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- supervisor/const.py
- supervisor/dbus/network/configuration.py
- supervisor/dbus/const.py
- tests/api/test_network.py
- supervisor/host/const.py
- tests/dbus_service_mocks/network_manager.py
- supervisor/api/network.py
- supervisor/dbus/network/setting/init.py
🔇 Additional comments (4)
supervisor/host/configuration.py (2)
58-59: LGTM! The new WifiConfig fields are well-typed.The addition of
bandandchannelfields to the WifiConfig dataclass is clean and properly typed withNoneas a possible value, making them optional.
196-196: LGTM! The SHARED method mapping is correctly implemented.The addition of the SHARED method mapping aligns with NetworkManager's interface method options.
supervisor/dbus/network/setting/generate.py (2)
18-19: LGTM! Good code organization and reusability.The new constants are properly defined, and the
_get_address_datahelper function effectively encapsulates the address data construction logic, improving code reusability and maintainability.Also applies to: 55-66
76-81: LGTM! The SHARED method handling is properly implemented.The IPv4 settings correctly handle the SHARED method, reusing the
_get_address_datahelper function for consistency.
jkt628
force-pushed
the
feat/ipv4-method-shared
branch
from
2b17e26 to
370a785
Compare
|
tested in HAOS, see home-assistant/home-assistant.io#37140 (comment) |
@agners please review home-assistant/home-assistant.io@37ca5de |
jkt628
force-pushed
the
feat/ipv4-method-shared
branch
from
370a785 to
5912941
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
supervisor/dbus/network/setting/generate.py (1)
55-66: Good refactoring! Consider similar refactoring for IPv6.The new
_get_address_datafunction effectively reduces code duplication. Similar logic exists in the IPv6 settings (lines 112-121) that could benefit from the same refactoring pattern.Apply this pattern to IPv6 settings:
+def _get_ipv6_address_data(ipv6setting) -> Variant: + address_data = [] + for address in ipv6setting.address: + address_data.append( + { + "address": Variant("s", str(address.ip)), + "prefix": Variant("u", int(address.with_prefixlen.split("/")[-1])), + } + ) + return Variant("aa{sv}", address_data) + def _get_ipv6_connection_settings(ipv6setting) -> dict: # ... existing code ... elif ipv6setting.method == InterfaceMethod.STATIC: ipv6[CONF_ATTR_IPV6_METHOD] = Variant("s", "manual") - address_data = [] - for address in ipv6setting.address: - address_data.append( - { - "address": Variant("s", str(address.ip)), - "prefix": Variant("u", int(address.with_prefixlen.split("/")[-1])), - } - ) - ipv6[CONF_ATTR_IPV6_ADDRESS_DATA] = Variant("aa{sv}", address_data) + ipv6[CONF_ATTR_IPV6_ADDRESS_DATA] = _get_ipv6_address_data(ipv6setting)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
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(4 hunks)supervisor/host/configuration.py(5 hunks)supervisor/host/const.py(2 hunks)tests/api/test_network.py(1 hunks)tests/dbus_service_mocks/network_manager.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- supervisor/const.py
- supervisor/dbus/network/configuration.py
- supervisor/dbus/const.py
- supervisor/api/network.py
- tests/api/test_network.py
- supervisor/host/const.py
- supervisor/dbus/network/setting/init.py
- supervisor/host/configuration.py
- tests/dbus_service_mocks/network_manager.py
🔇 Additional comments (2)
supervisor/dbus/network/setting/generate.py (2)
18-19: LGTM! Constants follow naming convention.The new constants for wireless band and channel configuration are well-placed and follow the existing naming pattern.
74-81: LGTM! Clean implementation of shared mode.The implementation of the SHARED interface method is consistent with other method handlers and correctly uses the refactored address data generation.
jkt628
force-pushed
the
feat/ipv4-method-shared
branch
from
dee53d5 to
65caf09
Compare
jkt628
force-pushed
the
feat/ipv4-method-shared
branch
from
65caf09 to
a8a4db2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
supervisor/dbus/network/setting/generate.py (1)
210-215: Simplify the mode fallback logic.The current ternary expression can be simplified using the
oroperator.- CONF_ATTR_802_WIRELESS_MODE: Variant( - "s", - interface.wifi.mode - if interface.wifi and interface.wifi.mode - else "infrastructure", - ), + CONF_ATTR_802_WIRELESS_MODE: Variant( + "s", + (interface.wifi and interface.wifi.mode) or "infrastructure", + ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
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(4 hunks)supervisor/host/configuration.py(5 hunks)supervisor/host/const.py(2 hunks)tests/api/test_network.py(1 hunks)tests/dbus_service_mocks/network_manager.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- supervisor/dbus/network/setting/init.py
- tests/api/test_network.py
- supervisor/dbus/const.py
- supervisor/dbus/network/configuration.py
- supervisor/api/network.py
- supervisor/host/configuration.py
- supervisor/const.py
- supervisor/host/const.py
- tests/dbus_service_mocks/network_manager.py
🔇 Additional comments (4)
supervisor/dbus/network/setting/generate.py (4)
18-19: LGTM! Constants follow naming convention.The new constants for WiFi band and channel configuration are properly named and organized.
55-66: Great refactoring! Helper function improves code modularity.The new
_get_address_datafunction:
- Eliminates code duplication between STATIC and SHARED methods
- Properly uses type hints
- Follows single responsibility principle
74-81: LGTM! IPv4 settings properly handle SHARED method.The implementation correctly supports the SHARED method required for hotspot functionality while maintaining consistency with other methods.
222-227: LGTM! Band and channel configuration properly implemented.The implementation:
- Correctly handles optional WiFi band and channel settings
- Properly wraps values in appropriate Variant types
- Maintains consistent error handling pattern with other WiFi settings
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
supervisor/dbus/network/setting/generate.py (1)
210-213: Simplify the mode assignment.The mode assignment can be simplified using the logical OR operator.
- CONF_ATTR_802_WIRELESS_MODE: Variant( - "s", - (interface.wifi.mode and interface.wifi.mode) or "infrastructure", - ), + CONF_ATTR_802_WIRELESS_MODE: Variant( + "s", + interface.wifi.mode or "infrastructure", + ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
supervisor/dbus/network/setting/generate.py(4 hunks)
🔇 Additional comments (4)
supervisor/dbus/network/setting/generate.py (4)
18-19: LGTM! Constants follow naming convention.The new constants for WiFi band and channel configuration are well-organized and follow the existing naming pattern.
55-66: Great refactoring! Extracted common address data logic.The new
_get_address_datafunction effectively reduces code duplication by extracting the common logic for generating address data. This improves maintainability and follows the DRY principle.
79-81: LGTM! Added support for shared IP method.The implementation correctly handles the shared IP method required for hotspot functionality, maintaining consistency with NetworkManager's expectations.
220-225: LGTM! Added WiFi band and channel configuration.The implementation correctly delegates band and channel validation to NetworkManager, which handles regulatory domain enforcement. This aligns with the separation of concerns principle discussed in the PR comments.
jkt628
force-pushed
the
feat/ipv4-method-shared
branch
2 times, most recently
from
072f768 to
935ab4b
Compare
- add IP method "shared" - new WiFi parameters "band" and "channel" NetworkManager provides the magic
use the actual error message from NetworkManager
jkt628
force-pushed
the
feat/ipv4-method-shared
branch
from
935ab4b to
3013108
Compare
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
Bug Fixes
Technical Updates