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

Tumble Dryer - Add Cycle State #32

Merged
merged 31 commits into from
Oct 29, 2021
Merged

Tumble Dryer - Add Cycle State #32

merged 31 commits into from
Oct 29, 2021

Conversation

terminet85
Copy link
Contributor

With this commit I've added the level state (DryLev) to the CandyTumbleStatusSensor. When DryerProgramState is STOPPED we set the reached dry level.

@codecov
Copy link

codecov bot commented Oct 25, 2021

Codecov Report

Merging #32 (eb081ef) into main (715cc38) will decrease coverage by 0.99%.
The diff coverage is 69.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #32      +/-   ##
==========================================
- Coverage   89.52%   88.52%   -1.00%     
==========================================
  Files           6        6              
  Lines         563      584      +21     
==========================================
+ Hits          504      517      +13     
- Misses         59       67       +8     
Impacted Files Coverage Δ
custom_components/candy/client/model.py 80.20% <63.15%> (-2.46%) ⬇️
custom_components/candy/sensor.py 98.85% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 715cc38...eb081ef. Read the comment docs.

Copy link
Owner

@ofalvai ofalvai left a comment

Choose a reason for hiding this comment

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

Left some comments about naming stuff properly.

Also, do we understand now what each dry level property means in practice? I'm not sure about the meaning based on the discussion in #5

@@ -234,7 +234,10 @@ def unique_id(self) -> str:
@property
def state(self) -> StateType:
status: TumbleDryerStatus = self.coordinator.data
return str(status.program_state)
if status.program_state in [DryerProgramState.STOPPED]:
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think it's a good idea to mix two properties in a single sensor. For other appliances, there are a separate sensor for the main machine state and eg. the wash program state.

Copy link
Owner

Choose a reason for hiding this comment

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

The other tumble dryer PR (#29) creates a separate sensor for the dry level, I think that's a better approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More sensors, more resources. And in this case, the tumble_cycle_status's sensor is almost useless because is overlapped by tumble_dryer.
It would be much more consistent, IMHO, to harmonize it to wash_cycle_status.

Copy link
Owner

Choose a reason for hiding this comment

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

Okay, I can see that it makes sense for the dryer that only has 3 program states (the washing machine has 10)

@@ -113,10 +113,33 @@ def __str__(self):
return "%s" % self


class DryerCycleState(Enum):
Copy link
Owner

Choose a reason for hiding this comment

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

If the values of this enum are different dry levels, I don't think DryerCycleState is a good name for the enum itself. I would suggest something like DryLevel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a cycle. Tumble dryer start is cycle from 1 and ends with the level set. IMHO that's more mnemonic.

Copy link
Owner

Choose a reason for hiding this comment

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

Makes sense, thanks!

@terminet85
Copy link
Contributor Author

Also, do we understand now what each dry level property means in practice? I'm not sure about the meaning based on the discussion in #5

I've done some tests. Starting dryer cycles and look API vars. What tumble dryer display says it's different from API and Candy's APP. For instance, when with API I reach DryLev 2 (Hang Dry), there is a mismatch with physical tumble display (reach Store Dry).
At same time if I open Candy APP, there is a popup that tell that I reach DryLev 2 (Hang Dry), and I can open tumble dryer door and going on with DryLev 3, restarting the machine. I've tried but I cannot do that.

Maybe others tumble dryer model, have this function, or there is a bug with this function. Anyway either API and Candy's APP are consistent, but physical machine's display show different -> this is a bug.

@ofalvai
Copy link
Owner

ofalvai commented Oct 29, 2021

Thanks for the explanation @terminet85, this is indeed weird. Let's stick to one interpretation of the values for now, then later we can iterate on it, maybe other owners chime in as well.

@ofalvai ofalvai merged commit e82dd7d into ofalvai:main Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants