Fix BRP069: Add swing_mode support for Alira X devices

[kingy444]

I am trying to fix some broken tests and found these missing.

Seem to be required for tests/test_daikin_alira_x.py and were removed at somepoint

I dont have one of these to test with, but assume the tests were written with these in mind

[fredrike]

Thank you for finding this!

This is the missing code: v2.11.1...v2.13.4#diff-92a2c94a05a479c70175c80e0a123bddeb645754f88ac774cb7b43b55dd5db10L180-L186

I think we should try to re-add it to the same place, after this:

pydaikin/pydaikin/daikin_brp069.py

Line 181 in 2debda5

if self.support_swing_mode:

but as param instead.

The code could look like this:

            if 'f_dir_lr' in self.values and 'f_dir_ud' in self.values:
                # Australian Alira X uses 2 separate parameters instead of the combined f_dir
                params["f_dir_ud"] = 'S' if self.values['f_dir'] in ('1', '3') else '0'
                params["f_dir_lr"] = 'S' if self.values['f_dir'] in ('2', '3') else '0'
            else:
                params["f_dir"] = self.values['f_dir']

I think this is nice but perhaps not clearer:

if all(i in self.values for i in ('f_dir_lr', 'f_dir_ud')):

[fredrike]

Suggested change

	if response.get("f_dir_ud") == "0" and response.get("f_dir_lr") == "0":
	response["f_dir"] = '0'
	if response.get("f_dir_ud") == "S" and response.get("f_dir_lr") == "0":
	response["f_dir"] = '1'
	if response.get("f_dir_ud") == "0" and response.get("f_dir_lr") == "S":
	response["f_dir"] = '2'
	if response.get("f_dir_ud") == "S" and response.get("f_dir_lr") == "S":
	response["f_dir"] = '3'
	f_dir = 0
	if response.get("f_dir_ud") == "S":
	f_dir += 1
	if response.get("f_dir_lr") == "S":
	f_dir += 2
	if "f_dir" not in response:
	response["f_dir"] = f_dir

[fredrike]

The idea is that S seems to add 1 or 2 to f_dir

[kel30a]

I think I find the previous version easier to understand, but I think the suggested change is equivalent.

[fredrike]

I agree but at least it should be elif

Suggested change

	if response.get("f_dir_ud") == "0" and response.get("f_dir_lr") == "0":
	response["f_dir"] = '0'
	if response.get("f_dir_ud") == "S" and response.get("f_dir_lr") == "0":
	response["f_dir"] = '1'
	if response.get("f_dir_ud") == "0" and response.get("f_dir_lr") == "S":
	response["f_dir"] = '2'
	if response.get("f_dir_ud") == "S" and response.get("f_dir_lr") == "S":
	response["f_dir"] = '3'
	if response.get("f_dir_ud") == "0" and response.get("f_dir_lr") == "0":
	response["f_dir"] = '0'
	elif response.get("f_dir_ud") == "S" and response.get("f_dir_lr") == "0":
	response["f_dir"] = '1'
	elif response.get("f_dir_ud") == "0" and response.get("f_dir_lr") == "S":
	response["f_dir"] = '2'
	elif response.get("f_dir_ud") == "S" and response.get("f_dir_lr") == "S":
	response["f_dir"] = '3'

[fredrike]

shorter but perhaps harder to read..

Suggested change

	if response.get("f_dir_ud") == "0" and response.get("f_dir_lr") == "0":
	response["f_dir"] = '0'
	if response.get("f_dir_ud") == "S" and response.get("f_dir_lr") == "0":
	response["f_dir"] = '1'
	if response.get("f_dir_ud") == "0" and response.get("f_dir_lr") == "S":
	response["f_dir"] = '2'
	if response.get("f_dir_ud") == "S" and response.get("f_dir_lr") == "S":
	response["f_dir"] = '3'
	if "f_dir" not in response:
	response["f_dir"] = (1 if response.get("f_dir_ud") == "S" else 0) + (
	2 if response.get("f_dir_lr") == "S" else 0
	)

[kel30a]

I think I prefer the more older, more explicit form

[fredrike]

This is also related: 1e77fc8

[fredrike]

@kel30a should also be included in this discussion.

[kel30a]

Tis a shame the pytest tests weren't previously running in the github workflows. I noticed @fredrike just added that in on master. Does @kingy444 need to bring those changes into his fork to make the test run as a "check" on this PR?

Happy to test once suggested changes have been made.

[kel30a]

I think I find the previous version easier to understand, but I think the suggested change is equivalent.

[kel30a]

I think I prefer the more older, more explicit form

[fredrike]

Tis a shame the pytest tests weren't previously running in the github workflows. I noticed @fredrike just added that in on master. Does @kingy444 need to bring those changes into his fork to make the test run as a "check" on this PR?

Happy to test once suggested changes have been made.

Well, BitBucket broke for us so I had to move the repository here and some info was lost in the transition.

The tests have not really been enforced but once we are back on track I'll enable them. I think a resync with master is a good thing here.

git fetch --all
git merge origin/master

[kingy444]

Enforcing tests might cause an issue until they are all sorted right?

There are other failing tests too that I haven't gotten around to fixing.

I did start but it's fun not having these devices.

I found the power tests are now asserting false where previously they were true...

Then even if I fake that assertion result the next bunch of checks fail too. Again, assuming that everything worked once upon a time I don't know whether the test is bad or the library 😂

[fredrike]

Well,having tests would probably have identified some of the errors we have introduced lately. But building good tests are hard too. I'm not sure what I think of this.

But great if we can sort the altaria issues for now.

[fredrike]

@kingy444 do you have time to fix this in the next couple of hours so I can release a new pydaikin version with the SkyFi fix together with this?

[kingy444]

@kingy444 do you have time to fix this in the next couple of hours so I can release a new pydaikin version with the SkyFi fix together with this?

trying to pull something together now for you