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 Iskra integration #121488

Merged
merged 23 commits into from
Sep 4, 2024
Merged

Conversation

iskrakranj
Copy link
Contributor

@iskrakranj iskrakranj commented Jul 8, 2024

Breaking change

Proposed change

Iskra, an energy meter producer from Slovenia, offers a range of high-quality products, including DIN rail mounted energy meters that are MID certified, measuring centers, and smart gateways. This integration allows users to easily integrate those device in home assistant.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

@iskrakranj iskrakranj changed the title Iskra integration Add Iskra integration Jul 8, 2024
@iskrakranj iskrakranj mentioned this pull request Jul 8, 2024
20 tasks
@iskrakranj iskrakranj marked this pull request as ready for review July 8, 2024 06:22
Copy link
Member

@joostlek joostlek left a comment

Choose a reason for hiding this comment

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

Integrations require full code coverage on the config flow. So please add unit tests for those, and preferably also for the other code.

@home-assistant home-assistant bot marked this pull request as draft July 8, 2024 06:26
@home-assistant
Copy link

home-assistant bot commented Jul 8, 2024

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

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

Hello @iskrakranj,

When attempting to inspect the commits of your pull request for CLA signature status among all authors we encountered commit(s) which were not linked to a GitHub account, thus not allowing us to determine their status(es).

The commits that are missing a linked GitHub account are the following:

Unfortunately, we are unable to accept this pull request until this situation is corrected.

Here are your options:

  1. If you had an email address set for the commit that simply wasn't linked to your GitHub account you can link that email now and it will retroactively apply to your commits. The simplest way to do this is to click the link to one of the above commits and look for a blue question mark in a blue circle in the top left. Hovering over that bubble will show you what email address you used. Clicking on that button will take you to your email address settings on GitHub. Just add the email address on that page and you're all set. GitHub has more information about this option in their help center.

  2. If you didn't use an email address at all, it was an invalid email, or it's one you can't link to your GitHub, you will need to change the authorship information of the commit and your global Git settings so this doesn't happen again going forward. GitHub provides some great instructions on how to change your authorship information in their help center.

    • If you only made a single commit you should be able to run
      git commit --amend --author="Author Name <[email protected]>"
      
      (substituting "Author Name" and "[email protected]" for your actual information) to set the authorship information.
    • If you made more than one commit and the commit with the missing authorship information is not the most recent one you have two options:
      1. You can re-create all commits missing authorship information. This is going to be the easiest solution for developers that aren't extremely confident in their Git and command line skills.
      2. You can use this script that GitHub provides to rewrite history. Please note: this should be used only if you are very confident in your abilities and understand its impacts.
    • Whichever method you choose, I will come by to re-check the pull request once you push the fixes to this branch.

We apologize for this inconvenience, especially since it usually bites new contributors to Home Assistant. We hope you understand the need for us to protect ourselves and the great community we all have built legally. The best thing to come out of this is that you only need to fix this once and it benefits the entire Home Assistant and GitHub community.

Thanks, I look forward to checking this PR again soon! ❤️

@iskrakranj iskrakranj force-pushed the iskra_integration branch from 357b828 to 1e69dfc Compare July 8, 2024 12:42
@iskrakranj iskrakranj marked this pull request as ready for review July 8, 2024 12:43
@home-assistant home-assistant bot requested a review from joostlek July 8, 2024 12:43
@home-assistant home-assistant bot dismissed their stale review July 8, 2024 12:43

Stale

@iskrakranj
Copy link
Contributor Author

@joostlek added config flow tests

CODEOWNERS Show resolved Hide resolved
Comment on lines 118 to 121
if user_input is not None:
self.host = user_input[CONF_HOST]
self.protocol = user_input[CONF_PROTOCOL]
if self.protocol == "Rest API":
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way we can detect the protocol by the host? How does the user know which one to pick?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't detect the connection method, but it depends on how the user is connecting Iskra's devices. If they use a smart gateway (WiFi to RS485/IR Modbus) to connect energy meters to Ethernet, they should use the REST API. However, if they use a Modbus RTU->TCP gateway or any Iskra measuring device that supports Ethernet connectivity, they should use Modbus TCP.

In summary:

  • REST API: meters connected to Smart gateway
  • Modbus TCP: meters connected to Modbus RTU->TCP gateway or Ethernet-enabled device

Copy link
Member

Choose a reason for hiding this comment

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

But can we probe and try the other one if it fails?

Copy link
Contributor Author

@iskrakranj iskrakranj Jul 22, 2024

Choose a reason for hiding this comment

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

@joostlek I can do something for sure but i'm not sure it would be okay. I can probe if the device responds to the REST API. If not, I can assume it is using Modbus TCP and prompt the user to enter the Modbus address and port. If that's acceptable, I can proceed with that approach.

However, we can't be certain it should use Modbus TCP in that case, as the user might just enter the wrong IP address.

We also support UDP broadcast discovery(not standard), which could later be added as a zeroconf method. This would work for all devices, except if the user sets the smart gateway to require authentication. In such cases, the user would need to enter credentials.

Copy link
Member

Choose a reason for hiding this comment

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

Zeroconf works with mDNS instead of UDP

Copy link
Member

Choose a reason for hiding this comment

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

I'll have a think about the rest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any updates?

return basic_info


class FlowHandler(ConfigFlow, domain=DOMAIN):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class FlowHandler(ConfigFlow, domain=DOMAIN):
class IskraConfigFlow(ConfigFlow, domain=DOMAIN):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

vol.Required(CONF_HOST): str,
vol.Required(CONF_PROTOCOL, default="Rest API"): selector.SelectSelector(
selector.SelectSelectorConfig(
options=["Rest API", "Modbus TCP"],
Copy link
Member

Choose a reason for hiding this comment

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

Let's at least pick snake_cased names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to snake_cased names

Comment on lines 59 to 63
else:
_LOGGER.error(
"Invalid protocol. Supported protocols are 'Modbus TCP' and 'Rest API'"
)
return False
Copy link
Member

Choose a reason for hiding this comment

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

This cant happen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed


@property
def device_info(self) -> DeviceInfo:
"""Return the device info of the Iskra Aqara device."""
Copy link
Member

Choose a reason for hiding this comment

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

iskra aqara?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

Comment on lines 117 to 118
@property
def device_info(self) -> DeviceInfo:
Copy link
Member

Choose a reason for hiding this comment

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

What entities does the gateway have?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently only number of connected devices, but i'will add more in follow up pull requests.
It also has pulse counter and temperature sensor user can use, tariff calculation and other status data.

Copy link
Member

Choose a reason for hiding this comment

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

Can't we see the amount of connected devices via the HA?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true we can, will delete this entity as it's unnecesary

return await hass.config_entries.async_unload_platforms(entry, PLATFORMS)


class IskraDevice(Entity):
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this to entity.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved

Comment on lines 197 to 191
# Add child devices if the root device is a gateway.
if root_device.is_gateway:
devices += root_device.get_child_devices()

# Add sensors for each device.
for device in devices:
sensors = []
coordinator = IskraDataUpdateCoordinator(hass, device)
await coordinator.async_config_entry_first_refresh()
Copy link
Member

Choose a reason for hiding this comment

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

Let's move the creation of coordinators to __init__.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 236 to 292
# Add resettable and non-resettable counters.
if device.supports_counters:
# Add Non-resettable counters.
for index, counter in enumerate(device.counters.non_resettable):

def non_resettable_value_func(device, counter_index):
return lambda device: round(
device.counters.non_resettable[counter_index].value, 2
)

sensor_entity_description = IskraSensorEntityDescription(
key=f"nresettable_counter{index+1}",
translation_key=f"nresettable_counter{index+1}",
state_class=SensorStateClass.TOTAL_INCREASING,
# If counter is for active energy mark it as energy sensor otherwise dont set device clas.
# This is due to the fact that HA does not support reactive energy sensors.
device_class=(
SensorDeviceClass.ENERGY
if counter.counter_type
in [CounterType.ACTIVE_IMPORT, CounterType.ACTIVE_EXPORT]
else None
),
# Use HA energy unit for active energy counters otherwise use units from API.
native_unit_of_measurement=(
UnitOfEnergy.WATT_HOUR
if counter.counter_type
in [CounterType.ACTIVE_IMPORT, CounterType.ACTIVE_EXPORT]
else counter.units
),
value_func=non_resettable_value_func(device, index),
)
entities.append(
IskraSensor(
coordinator, root_device, entry, sensor_entity_description
)
)

# Add resettable counters.
for index, counter in enumerate(device.counters.resettable):

def resettable_value_func(device, counter_index):
return lambda device: round(
device.counters.resettable[counter_index].value, 2
)

sensor_entity_description = IskraSensorEntityDescription(
key=f"resettable_counter{index+1}",
translation_key=f"resettable_counter{index+1}",
state_class=SensorStateClass.TOTAL_INCREASING,
# If counter is for active energy mark it as energy sensor otherwise dont set device clas.
# This is due to the fact that HA does not support reactive energy sensors.
device_class=(
SensorDeviceClass.ENERGY
if counter.counter_type
in [CounterType.ACTIVE_IMPORT, CounterType.ACTIVE_EXPORT]
else None
),
# Use HA energy unit for active energy counters otherwise use units from API.
native_unit_of_measurement=(
UnitOfEnergy.WATT_HOUR
if counter.counter_type
in [CounterType.ACTIVE_IMPORT, CounterType.ACTIVE_EXPORT]
else counter.units
),
value_func=resettable_value_func(device, index),
)

entities.append(
IskraSensor(
coordinator, root_device, entry, sensor_entity_description
)
)
Copy link
Member

Choose a reason for hiding this comment

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

This looks very complex, can we maybe move this to a follow up so we can focus on the normal devices first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but this is a crucial aspect of our measuring devices. It is a bit complicated, so let me explain it further.

All our devices, including energy meters and power quality analyzers, have energy counters that measure electrical energy. These devices contain a few non-resettable energy counters, which users cannot reset, and a few resettable ones. This is why there are two for loops in the code.

Resettable Counters

  • Customizable by User: Users can configure each resettable counter to measure:

    • Imported or exported energy
    • Active, reactive, or apparent energy
    • Total energy or energy for a specific phase of the meter
  • Example Use Case: This customization can be particularly handy. For instance, a user can set a load such as a central heat pump on one phase and a sanitary heat pump on another phase. They can then see the consumption of each load with one energy meter, as well as the total consumption for the whole heating system.

Non-resettable Counters

  • Customizable by Buyer: Larger companies or distributors that purchase from us can customize these counters. However, users cannot reconfigure them during the product's lifetime.

The code checks if the counter type is active energy and, if so, sets the device class to Home Assistant's SensorDeviceClass.Energy and the unit of energy to Watt_HOUR. Otherwise, it uses the API's units. This ensures compatibility with the Energy tab in Home Assistant.

Copy link
Member

Choose a reason for hiding this comment

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

At this moment it's still a blur for me as the code isn't that readable and I can't completely link to whats going on. I don't think we can make 2024.8 (for which the beta cut is wednesday), so let's aim for 2024.9. so in theory we have enough time and I think it would speed up the process to get the way we fetch data and how we create entities straight in this PR and add the extra logic in followups (and then we can do it in understandable bit size pieces). This way we can go over each subject and decide on the best way to do it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joostlek so how do i proceed. should i remove this part of code?

Copy link
Member

Choose a reason for hiding this comment

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

For now I think that's best yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok removed this part

@home-assistant home-assistant bot marked this pull request as draft July 19, 2024 11:35
Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

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

Hello @iskrakranj,

When attempting to inspect the commits of your pull request for CLA signature status among all authors we encountered commit(s) which were not linked to a GitHub account, thus not allowing us to determine their status(es).

The commits that are missing a linked GitHub account are the following:

Unfortunately, we are unable to accept this pull request until this situation is corrected.

Here are your options:

  1. If you had an email address set for the commit that simply wasn't linked to your GitHub account you can link that email now and it will retroactively apply to your commits. The simplest way to do this is to click the link to one of the above commits and look for a blue question mark in a blue circle in the top left. Hovering over that bubble will show you what email address you used. Clicking on that button will take you to your email address settings on GitHub. Just add the email address on that page and you're all set. GitHub has more information about this option in their help center.

  2. If you didn't use an email address at all, it was an invalid email, or it's one you can't link to your GitHub, you will need to change the authorship information of the commit and your global Git settings so this doesn't happen again going forward. GitHub provides some great instructions on how to change your authorship information in their help center.

    • If you only made a single commit you should be able to run
      git commit --amend --author="Author Name <[email protected]>"
      
      (substituting "Author Name" and "[email protected]" for your actual information) to set the authorship information.
    • If you made more than one commit and the commit with the missing authorship information is not the most recent one you have two options:
      1. You can re-create all commits missing authorship information. This is going to be the easiest solution for developers that aren't extremely confident in their Git and command line skills.
      2. You can use this script that GitHub provides to rewrite history. Please note: this should be used only if you are very confident in your abilities and understand its impacts.
    • Whichever method you choose, I will come by to re-check the pull request once you push the fixes to this branch.

We apologize for this inconvenience, especially since it usually bites new contributors to Home Assistant. We hope you understand the need for us to protect ourselves and the great community we all have built legally. The best thing to come out of this is that you only need to fix this once and it benefits the entire Home Assistant and GitHub community.

Thanks, I look forward to checking this PR again soon! ❤️

config_entry_id=entry.entry_id,
identifiers={(DOMAIN, base_device.serial)},
manufacturer=MANUFACTURER,
name=f"{base_device.serial}",
Copy link
Member

Choose a reason for hiding this comment

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

is this a string or an int?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'll use it directly, same for deviceInfo in iskra entity

Comment on lines 154 to 159
authentication = {
"username": user_input[CONF_USERNAME],
"password": user_input[CONF_PASSWORD],
}
try:
device_info = await test_rest_api_connection(self.host, authentication)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
authentication = {
"username": user_input[CONF_USERNAME],
"password": user_input[CONF_PASSWORD],
}
try:
device_info = await test_rest_api_connection(self.host, authentication)
try:
device_info = await test_rest_api_connection(self.host, user_input)

user_input is already shaped like that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed as suggested + to simplify changed _create_entry function

super().__init__(coordinator)
device = coordinator.device
gateway = device.parent_device
device_name = f"{device.serial}"
Copy link
Member

Choose a reason for hiding this comment

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

is device.serial a number or string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

string, will use directly

class IskraEntity(CoordinatorEntity[IskraDataUpdateCoordinator]):
"""Representation a base Iskra device."""

_attr_should_poll = True
Copy link
Member

Choose a reason for hiding this comment

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

why do we poll?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i see Entity class has it by default, removed

Comment on lines 209 to 215
entities.extend(
[
IskraSensor(coordinator, description)
for description in SENSOR_TYPES
if description.key in sensors
]
)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
entities.extend(
[
IskraSensor(coordinator, description)
for description in SENSOR_TYPES
if description.key in sensors
]
)
entities.extend(
IskraSensor(coordinator, description)
for description in SENSOR_TYPES
if description.key in sensors
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed []

Comment on lines 8 to 16
"host": "Host"
}
},
"authentication": {
"title": "Configure Rest API Credentials",
"description": "Enter username and password",
"data": {
"username": "Username",
"password": "Password"
Copy link
Member

Choose a reason for hiding this comment

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

Please use common strings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

"description": "Enter Modbus TCP port and device's Modbus address.",
"data": {
"port": "Port",
"address": "Modbus address"
Copy link
Member

Choose a reason for hiding this comment

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

What's a modbus address?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a device's address inside Modbus network it must be unique inside network, You could think of it like a simplified IP address, its a 1 byte number. User can set it on device and it's required to communicate with device with Modbus (part of modbus packet)

Copy link
Member

Choose a reason for hiding this comment

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

How can the user know what to fill in?

Copy link
Contributor Author

@iskrakranj iskrakranj Sep 4, 2024

Choose a reason for hiding this comment

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

This is set on device, user can navigate through user menu on device's LCD and set this settings but by default port is 10001 and modbus address 33

Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to add a data_description thing where we explain where the user can find these values?

# Frequency
ATTR_FREQUENCY = "frequency"

TIMEOUT = 60
Copy link
Member

Choose a reason for hiding this comment

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

unused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@home-assistant home-assistant bot marked this pull request as draft September 3, 2024 13:07
homeassistant/components/iskra/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/iskra/__init__.py Outdated Show resolved Hide resolved
tests/components/iskra/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/iskra/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/iskra/test_config_flow.py Outdated Show resolved Hide resolved
# Test successful Rest API configuration
assert result["type"] is FlowResultType.CREATE_ENTRY
assert result["title"] == f"{SG_MODEL} {SERIAL}"
assert result["data"][CONF_HOST] == HOST
Copy link
Member

Choose a reason for hiding this comment

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

idem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added full data dict asserts

Copy link
Member

Choose a reason for hiding this comment

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

Also change this to the way I explained above

# Test successful Modbus TCP configuration
assert result["type"] is FlowResultType.CREATE_ENTRY
assert result["title"] == f"{PQ_MODEL} {SERIAL}"
assert result["data"][CONF_HOST] == HOST
Copy link
Member

Choose a reason for hiding this comment

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

idem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added full data dict asserts

Copy link
Member

Choose a reason for hiding this comment

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

idem

Comment on lines 165 to 173
async def test_abort_if_already_setup(hass: HomeAssistant, mock_pyiskra_rest) -> None:
"""Test we abort if Iskra is already setup."""

MockConfigEntry(domain="iskra", unique_id=SERIAL).add_to_hass(hass)
result = await hass.config_entries.flow.async_init(
"iskra",
context={"source": SOURCE_USER},
data={CONF_HOST: HOST, CONF_PROTOCOL: "rest_api"},
)
Copy link
Member

Choose a reason for hiding this comment

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

should we parametrize this check to also check if this is checked when we go for modbus?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

parametrized

Comment on lines 201 to 203
assert result["type"] is FlowResultType.FORM
assert result["errors"] == {"base": reason}
assert result["step_id"] == "user"
Copy link
Member

Choose a reason for hiding this comment

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

Tests should end in either create_entry or abort. So let's repatch this test and finalize it so we also test this is testing that the flow is able to end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I parametrized this test so it's same for restapi and modbus. I Also changed test name as it was misleading, i wanted to test if test returns error, which doesn't cause abort. Should i add code that after error is tested to proceed with normal error free creation of the device?

Copy link
Member

Choose a reason for hiding this comment

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

Yes! This way we test that the flow is able to recover and eliminate the possibility of introducing a stateful bug where that can't happen anymore

Comment on lines 238 to 241
# Test if aborted
assert result["type"] is FlowResultType.FORM
assert result["errors"] == {"base": reason}
assert result["step_id"] == "modbus_tcp"
Copy link
Member

Choose a reason for hiding this comment

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

idem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

@iskrakranj iskrakranj marked this pull request as ready for review September 4, 2024 09:17
@home-assistant home-assistant bot requested a review from joostlek September 4, 2024 09:17
Copy link
Member

@joostlek joostlek left a comment

Choose a reason for hiding this comment

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

I think after this review it should be ready to merge :)

Comment on lines 194 to 195
if user_input[CONF_ADDRESS] not in range(1, 256):
errors["base"] = "address_range"
Copy link
Member

Choose a reason for hiding this comment

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

You just added validation for the port, should we do the same for the address so we can elimate this check?

Copy link
Contributor Author

@iskrakranj iskrakranj Sep 4, 2024

Choose a reason for hiding this comment

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

At first i used range in voluptuous scheme but what happened is HA changed it to slider instead of input field.
it works ok for larger numbers is there a way to set it to display input field?

Copy link
Member

Choose a reason for hiding this comment

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

Euh yes there is, it's a NumberSelector, let me check

Copy link
Member

@joostlek joostlek Sep 4, 2024

Choose a reason for hiding this comment

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

            vol.Optional(
                CONF_TEMPERATURE,
                description={"suggested_value": options.get(CONF_TEMPERATURE)},
                default=RECOMMENDED_TEMPERATURE,
            ): NumberSelector(NumberSelectorConfig(min=0, max=1, step=0.05, mode=NumberSelectorMode.BOX,)),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay this works, i changed it so it uses Number selector but it returns float so i had to add conversion to integer hope that's ok

homeassistant/components/iskra/config_flow.py Outdated Show resolved Hide resolved
serial = device_info.serial
model = device_info.model

if not self.unique_id:
Copy link
Member

Choose a reason for hiding this comment

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

unique_id is not set at this point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed condition

self._abort_if_unique_id_configured()

return self.async_create_entry(
title=f"{model} {serial}",
Copy link
Member

Choose a reason for hiding this comment

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

Should we just do the model? We can add the serial number to the device_info so the user can recognize the device, but putting a full serial number in the name isn't user friendly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree will change to just model

identifiers={(DOMAIN, device.serial)},
manufacturer=MANUFACTURER,
model=device.model,
name=device.serial,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
name=device.serial,

If you omit the name of a device, it will take the name of the config entry, so this will save the user renaming more stuff

Copy link
Contributor Author

@iskrakranj iskrakranj Sep 4, 2024

Choose a reason for hiding this comment

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

nice, done it only for devices that doesn't have gateway of course as suggested

# Test successful Rest API configuration
assert result["type"] is FlowResultType.CREATE_ENTRY
assert result["title"] == f"{SG_MODEL} {SERIAL}"
assert result["data"][CONF_HOST] == HOST
Copy link
Member

Choose a reason for hiding this comment

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

assert result["result"].unique_id == "..."

# Test successful Rest API configuration
assert result["type"] is FlowResultType.CREATE_ENTRY
assert result["title"] == f"{SG_MODEL} {SERIAL}"
assert result["data"][CONF_HOST] == HOST
Copy link
Member

Choose a reason for hiding this comment

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

Also change this to the way I explained above

# Test successful Modbus TCP configuration
assert result["type"] is FlowResultType.CREATE_ENTRY
assert result["title"] == f"{PQ_MODEL} {SERIAL}"
assert result["data"][CONF_HOST] == HOST
Copy link
Member

Choose a reason for hiding this comment

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

idem

Comment on lines 149 to 180
@pytest.mark.parametrize(
("protocol"),
[
("rest_api"),
("modbus_tcp"),
],
)
async def test_abort_if_already_setup(
hass: HomeAssistant, mock_pyiskra_rest, mock_pyiskra_modbus, protocol
) -> None:
"""Test we abort if Iskra is already setup."""

MockConfigEntry(domain=DOMAIN, unique_id=SERIAL).add_to_hass(hass)
result = await hass.config_entries.flow.async_init(
DOMAIN,
context={"source": SOURCE_USER},
data={CONF_HOST: HOST, CONF_PROTOCOL: protocol},
)

if protocol == "modbus_tcp":
assert result["type"] is FlowResultType.FORM
assert result["step_id"] == "modbus_tcp"
result = await hass.config_entries.flow.async_configure(
result["flow_id"],
user_input={
CONF_PORT: MODBUS_PORT,
CONF_ADDRESS: MODBUS_ADDRESS,
},
)

assert result["type"] is FlowResultType.ABORT
assert result["reason"] == "already_configured"
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if I told this test to be parametrized, but we should not have if statements in the code

Copy link
Member

Choose a reason for hiding this comment

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

Maybe I forgot that it required an extra step, if that's the case, sorry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

np, changed it back same for error test

Comment on lines 183 to 212
@pytest.mark.parametrize(
("protocol", "s_effect", "reason"),
[
("rest_api", DeviceConnectionError, "cannot_connect"),
("rest_api", DeviceTimeoutError, "cannot_connect"),
("rest_api", InvalidResponseCode, "cannot_connect"),
("rest_api", Exception, "unknown"),
("modbus_tcp", DeviceConnectionError, "cannot_connect"),
("modbus_tcp", DeviceTimeoutError, "cannot_connect"),
("modbus_tcp", InvalidResponseCode, "cannot_connect"),
("modbus_tcp", Exception, "unknown"),
],
)
async def test_device_error(
hass: HomeAssistant,
mock_pyiskra_rest,
mock_pyiskra_modbus,
protocol,
s_effect,
reason,
) -> None:
"""Test device error with Modbus TCP protocol."""
mock_pyiskra_modbus.side_effect = s_effect
mock_pyiskra_rest.side_effect = s_effect

result = await hass.config_entries.flow.async_init(
DOMAIN,
context={"source": SOURCE_USER},
data={CONF_HOST: HOST, CONF_PROTOCOL: protocol},
)
Copy link
Member

Choose a reason for hiding this comment

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

same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

@home-assistant home-assistant bot marked this pull request as draft September 4, 2024 10:57
@iskrakranj iskrakranj marked this pull request as ready for review September 4, 2024 12:17
@home-assistant home-assistant bot requested a review from joostlek September 4, 2024 12:17
@joostlek
Copy link
Member

joostlek commented Sep 4, 2024

Let's run the CI

If that works, we can merge it

@joostlek
Copy link
Member

joostlek commented Sep 4, 2024

pylint is failing on the tests

@iskrakranj
Copy link
Contributor Author

Fixed lint error

@joostlek joostlek merged commit b557e9e into home-assistant:dev Sep 4, 2024
40 of 42 checks passed
@joostlek
Copy link
Member

joostlek commented Sep 4, 2024

Do you have discord?

@iskrakranj
Copy link
Contributor Author

sorry i don't blocked by IT

@iskrakranj
Copy link
Contributor Author

but i have personal one StarKiller99#1716

@joostlek
Copy link
Member

joostlek commented Sep 4, 2024

The #'s have been removed a while back. If you join the HA discord server we can add you to the integration owner channel so you can stay up to date

@iskrakranj
Copy link
Contributor Author

didnt know heh, i just joined

iloveicedgreentea pushed a commit to iloveicedgreentea/core that referenced this pull request Sep 4, 2024
* Add iskra integration

* iskra non resettable counters naming fix

* added iskra config_flow test

* fixed iskra integration according to code review

* changed iskra config flow test

* iskra integration, fixed codeowners

* Removed counters code & minor fixes

* added comment

* Update homeassistant/components/iskra/__init__.py

Co-authored-by: Joost Lekkerkerker <[email protected]>

* Updated Iskra integration according to review

* Update homeassistant/components/iskra/strings.json

Co-authored-by: Joost Lekkerkerker <[email protected]>

* Updated iskra integration according to review

* minor iskra integration change

* iskra integration changes according to review

* iskra integration changes according to review

* Changed iskra integration according to review

* added iskra config_flow range validation

* Fixed tests for iskra integration

* Update homeassistant/components/iskra/coordinator.py

* Update homeassistant/components/iskra/config_flow.py

Co-authored-by: Joost Lekkerkerker <[email protected]>

* Fixed iskra integration according to review

* Changed voluptuous schema for iskra integration and added data_descriptions

* Iskra integration tests lint error fix

---------

Co-authored-by: Joost Lekkerkerker <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Sep 5, 2024
@iskrakranj iskrakranj deleted the iskra_integration branch September 23, 2024 10:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants