Device connection type

[chemelli74]

Add a method to know the device connection type, none if not able to establish one.

cc @mib1185

[kbr]

Thank you for providing this, but in my opinion this code should not make it into the core part. The core part just provides the communication with the device but not the interpretation of the data. The code should be at application level or, may be, in the library; FritzStatus could be an option. Also it would be good to know what kind of "DefaultConnectionService" return values are provided by different router models. For my DSL connection its also "1.WANPPPConnection.1" but there are other router models for WAN-access by cable, fibre and LTE/G5. In lack of those models I don't know about possible differences and we should "refuse the temptation to guess" – at least for coding the library.

Also for new code we need according tests (even if it is just mocking). Because a test would show that this code will break with Python < 3.8 (and just slicing like [2:-2] will do the same as [2:][:-2], but more efficient.)

[chemelli74]

Thank you for providing this, but in my opinion this code should not make it into the core part. The core part just provides the communication with the device but not the interpretation of the data. The code should be at application level or, may be, in the library; FritzStatus could be an option.

TBH, I was in doubt where to place such feature. Happy to move to FritzStatus.

Also it would be good to know what kind of "DefaultConnectionService" return values are provided by different router models. For my DSL connection its also "1.WANPPPConnection.1" but there are other router models for WAN-access by cable, fibre and LTE/G5. In lack of those models I don't know about possible differences and we should "refuse the temptation to guess" – at least for coding the library.

The reply cloud be "1.WANPPPConnection.1" or "1.WANIPConnection.1" (source: https://www.broadband-forum.org/download/TR-133_Corrigendum-1.pdf) The code already use it dynamically for the next call_action.

Also for new code we need according tests (even if it is just mocking). Because a test would show that this code will break with Python < 3.8 (and just slicing like [2:-2] will do the same as [2:][:-2], but more efficient.)

Indeed tests are needed, I was just waiting to agree on the code itself before investing time in writing them ;-)

Simone

[chemelli74]

@kbr, conflict fixed, ready for merging ;-)

[kbr]

I'm a bit confused about https://github.com/kbr/fritzconnection/pull/146/commits/f1a152c60f1735218155e1eb42bf3e14b005c983 This is already merged as #153

Regarding https://github.com/kbr/fritzconnection/pull/146/commits/efb83667166a7bcca42e7274d22f92f674eb4ffe :

This will not run with Python <= 3.7. Even if there is no test for this method, running the tests would show this because the compiler will stop with a syntax error. A more pythonic way would be to call the service directly and delegate the error handling to the application:

    @property
    def connection_type(self):
        result = self.fc.call_action(
                "Layer3Forwarding1", "GetDefaultConnectionService"
        )
        return result["NewDefaultConnectionService"]

This will return an unstripped string like "1.WANPPPConnection.1" providing the returned value from the router "as is". Alternative this could get implemented as a method with a parameter about returning the raw or stripped result:

    def get_connection_type(self, raw=True):
        result = self.fc.call_action(
                "Layer3Forwarding1", "GetDefaultConnectionService"
        )
        connection_type = result["NewDefaultConnectionService"]
        if not raw:
            # stript leading and tailing dot and number
            connection_type = connection_type.split('.')[1]
        return connection_type

So the application can decide about the desired return value. But after thinking about this feature a second time, I prefer the property-version delegating the interpretation to the application.

[chemelli74]

But after thinking about this feature a second time, I prefer the property-version delegating the interpretation to the application.

The idea is to have a string ready for a subsequent call_action, so please rethink about it and allow the parsing ;-)

Simone

[chemelli74]

Just to clarify: I think the idea with the raw parameter is really winning. As you can achive both goals with it.

Simone

[kbr]

The point is, that I don't know why the "dotted" format gets returned. It must have some meaning, otherwise it would be a stupid format. So I hesitate to interpret the result inside the library without proper knowledge, even if it would work for a subsequent call. It's kind of pureism vs. pragmatism (the latter potentially dirty – and I don't like dirty code ;)

[chemelli74]

Let me find more info about it then, in the mean time you can merge the other ones ? :-)

Simone

[chemelli74]

As far as I can get it I would say the schema is:

<device number>.<device connection>.<device interface> => 1.WANPPPConnection.1.

Some more details: https://github.com/RouterHAK/CPE-Models/blob/main/GR_Cosmote_OXYGEN_OxyGEN-Router.json

[chemelli74]

Code updated

[chemelli74]

Can we kindly progress with this PR please ?

Thx in advance,

Simone

[kbr]

As far as I can get it I would say the schema is:

<device number>.<device connection>.<device interface> => 1.WANPPPConnection.1.

Some more details: https://github.com/RouterHAK/CPE-Models/blob/main/GR_Cosmote_OXYGEN_OxyGEN-Router.json

The link provides a lot of cases with the same style, but where do you got the information about device_number and device_interface?

As long as this is unclear the values should not have dedicated names but more generic ones like prefix and postfix (which they are).

An according method could be get_default_connection_service() returning the router-DefaultConnectionService:

    def get_default_connection_service(self):
        """
        Returns a namedtuple of three values interpreted as 
        `prefix` -> int
        `device_connection_service` -> str (like "WANPPPConnection")
        `postfix` -> int
        """
        result = self.fc.call_action(
                "Layer3Forwarding1", "GetDefaultConnectionService"
        )
        device_number, device_connection, device_interface = \
            result["NewDefaultConnectionService"].split('.')
        DefaultConnectionService = namedtuple(
            "DefaultConnectionService", 
            "prefix device_connection postfix"
        )
        return DefaultConnectionService(
            int(prefix), device_connection_service, int(postfix)
        )

[chemelli74]

Can we avoid raising an error if Layer3Forwarding1 is not existing ? We can return "0, None, 0"

[kbr]

Can we avoid raising an error if Layer3Forwarding1 is not existing ? We can return "0, None, 0"

FritzConnection works in a way in case a user called something involving a nonexisting Service a FritzServiceError gets raised. The library should be consistent keeping EAFP.

One have to check for None or an exception anyway. Yes, I know None can get used directly in a boolean context for i.e. a shortcut, but None can also hide other unexpected errors. The answer can be different on application level – but the library should avoid this.

[chemelli74]

Can we avoid raising an error if Layer3Forwarding1 is not existing ? We can return "0, None, 0"

FritzConnection works in a way in case a user called something involving a nonexisting Service a FritzServiceError gets raised. The library should be consistent keeping EAFP.

One have to check for None or an exception anyway. Yes, I know None can get used directly in a boolean context for i.e. a shortcut, but None can also hide other unexpected errors. The answer can be different on application level – but the library should avoid this.

Ok, will make the check at application level then.

Code updated.

Simone

[chemelli74]

@kbr do you think is now good to go ? :-)

Simone

[chemelli74]

Ping :-)

Simone

[chemelli74]

Hi Klaus, in the hope that all is fine on your side and without any pressure, I would like to ask if you have an idea of when you'll have some time to progress with the pending PR ?

Thank you in advance,

Simone

[kbr]

Hi Simone,

thanks for asking. After some delay there are busy days again and also I will have a short, but necessary timeout. Hopefully starting in June I will be able to head on for the next release :-)

Klaus

[chemelli74]

Hi Klaus, any raft idea on a timeframe for a new release with the new features and enhancements ?

Thx,

Simone

[kbr]

Hi Simone, thanks to keep in touch. Hope to start again soon.

[kbr]

Hi Simone, due to the time passed I have some distance to the former code and a fresh view (i.e. starting with with version 1.10 I will add a new develoment branch for pull request, so work can continue there without disrupting the master-branch; that will also get documented). So far I suggest the following changes:

• Rename device_support_meshto device_has_mesh_support as it is a boolean property and adapt the comment according to an attribute:

@property
def device_has_mesh_support(self):
    """
    True if the device supports mesh, otherwise False.
    """
    # check for the corresponding action
    # whether mesh is supported
    try:
        return (
            "X_AVM-DE_GetMeshListPath"
            in self.fc.services["Hosts1"].actions
        )
    except KeyError:
        # can happen if "Hosts1" is not known
        return False

• Simplify the get_default_connection_service() method and keep it as a method (not a property):

def get_default_connection_service(self):
    """
    Returns a namedtuple of type DefaultConnectionService:
    `prefix` -> str
    `device_connection` -> str (like "WANPPPConnection")
    `postfix` -> str
    """
    result = self.fc.call_action(
            "Layer3Forwarding1", "GetDefaultConnectionService"
    )
    prefix, connection_service, postfix = \
        result["NewDefaultConnectionService"].split('.', 2)
    return DefaultConnectionService(
        prefix, connection_service, postfix
    )

• The DefaultConnectionService type should get defined at module level:

DefaultConnectionService = namedtuple(
    "DefaultConnectionService",
    "prefix connection_service postfix"
)

• Access to the relevant information (like "WANPPPConnection") can then get simplified by a new property connection_service:

@property
def connection_service(self):
    """
    The connection service provided by the device (i.e "WANPPPConnection")
    """
    return self.get_default_connection_service().connection_service

[chemelli74]

Updated and ready ;-)

Simone