Implement get_device_info for F5359 used by KPN Box 12

[rhpijnacker]

Getting the value for Path Device/DeviceInfo triggers the UnknownPathException on a F5359 device.
The required information is available via individual requests, though.

[rhpijnacker]

An obvious improvement is to create one combined request for all of these values.
As a first start and to trigger the discussion I am sending the pull request like this.

[iMicknl]

An obvious improvement is to create one combined request for all of these values. As a first start and to trigger the discussion I am sending the pull request like this.

Thanks! I wonder why this doesn't work while pulling Device Info.

Wouldn't it make sense to catch this exception in the HA integration and execute the different commands?
(thus move the logic from this PR to ha-sagemcom)

[rhpijnacker]

Wouldn't it make sense to catch this exception in the HA integration and execute the different commands?
(thus move the logic from this PR to ha-sagemcom)

I do not think so. Conceptually this still returns the device info, so seems to fit well where I put it.
I'm not sure about some of the fields, though. Do you have an example of what it looks like for another device?

[rhpijnacker]

I wonder why this doesn't work while pulling Device Info.

I would not know either. We can try to raise a bug report at KPN, but I'm not optimistic that will lead anywhere.

[iMicknl]

Thanks @rhpijnacker, I will try to look at this early next week. Would be good if it could be merged and released :).

[rhpijnacker]

@iMicknl To clean up things, I also implemented a single-call with multiple actions interface. I just did not yet create a PR for that. You may want to have a look at that too: https://github.com/rhpijnacker/python-sagemcom-api/tree/feature/f5359-device-info-2. I could merge this if you think this is an improvement.

[rhpijnacker]

@iMicknl

Would be good if it could be merged and released :).

That would be nice indeed!
With the X-mas holidays coming up, I'd have time to do some rework (if needed).
I'm eagerly anticipating your comments.

[iMicknl]

@rhpijnacker sorry I didn't review this one yet, since CI is failing. Do you know how to run pre-commit locally? This will help you with the styling etc.

Otherwise I can help out and merge it afterwards.

[rhpijnacker]

@iMicknl No, I do not know how to run the pre-commit locally (yet). It looks like there are not git pre-commit hooks in place.
I was waiting on feedback first on whether or not this is the right direction in the first place.

[iMicknl]

PR is fine by me! However, the login/logout part is in this PR as well.

Could you reply on the logout pr? If we can agree on that one, I will merge both and release :).

[rhpijnacker]

Yeah, I think I committed to the wrong branch at some point, which explains why the logout part is in here too.

Before releasing this, you may want to take a look at rhpijnacker@55adf7f too. This combines the call to get the device info in one rest call, instead of doing multiple.

I'm new to Python too, so if you have suggestions how this could be done better, that would be well appreciated.

[iMicknl]

Before releasing this, you may want to take a look at rhpijnacker@55adf7f too. This combines the call to get the device info in one rest call, instead of doing multiple.

This would be a good improvement for a follow up PR. As you see, I am a bit late with replies lately since I am quite busy with bringing another integration to Home Assistant core. (and other things outside HA :D).

I will merge this one now and see if we need something more, otherwise I can release it.

[iMicknl]

In a follow up PR, it would be good to try catch this as well, since this could also throw an UnknownPathException.

[rhpijnacker]

What do you propose to do if this also throws an exception?
I do not see any way to recover from that, so why not just let the exception propagate?

[iMicknl]

You are right, we need to catch this in the Home Assistant integration :-)