Feature request: Allow setting custom connection timeout in `SMPClient()`

[gudnimg]

When using this code:

async with SMPClient(SMPBLETransport(), "00:11:22:33:44:55") as client:
    # do stuff

the timeout on the BLE scan when using an address is only 5 seconds and I don't see any way to modify it. 5 seconds is awfully short for BLE devices which have slow advertising intervals and when the environment has a lot of Bluetooth traffic. The specification allows the advertising interval to go up to 10 seconds.

I'm mainly concerned about the connect call which isn't directly called by my own script:

async def __aenter__(self) -> "SMPClient":
        await self.connect()
        return self

I am open to submitting a PR for this but want to ask first if the maintainers are open to this idea. :)

[gudnimg]

I have something like this in mind. I think it is backwards compatible. 🤔

Edit:
No its not backwards compatible if one calls connect directly and used the timeout parameter. Probably better to keep it.

[JPHutchins]

@gudnimg This is definitely is a problem. There's an architectural issue here, that we're working on with these breaking changes: #58

@GHF Had what I think is a good idea of removing the connect and disconnect method from the client entirely: #58 (comment)

The idea is that an smpclient exists in the context of a connected client and does not have to worry about the transport connect/disconnect effects.

Anyway, all transports accept a timeout parameter:

smpclient/smpclient/transport/__init__.py

Lines 12 to 21 in dcace1a

	class SMPTransport(Protocol):
	_smp_server_transport_buffer_size: int | None = None
	"""The SMP server transport buffer size, in 8-bit bytes."""
	async def connect(self, address: str, timeout_s: float) -> None: # pragma: no cover
	"""Connect the `SMPTransport`.
	Args:
	address: The SMP server address.
	timeout_s: The connection timeout in seconds."""

And then the client passes that to the transport:

smpclient/smpclient/__init__.py

Lines 95 to 102 in dcace1a

	async def connect(self, timeout_s: float = 5.0) -> None:
	"""Connect to the SMP server.
	Args:
	timeout_s: the timeout for the connection attempt in seconds
	"""
	await self._transport.connect(self._address, timeout_s)
	await self._initialize()

AFAICT you wouldn't be able to use the (existing) async context manager, as you've observed that it calls connect without arguments 😢.

Looks like smpmgr implemented this with wait_for, probably done before the timeout arg was added:

https://github.com/intercreate/smpmgr/blob/6651700ec538284036371602b5cafa1e2ebae94c/smpmgr/main.py#L147

https://github.com/intercreate/smpmgr/blob/6651700ec538284036371602b5cafa1e2ebae94c/smpmgr/common.py#L83-L91

[JPHutchins]

@gudnimg A good, non breaking, intermediate improvement may be to add a new asynccontextmanager. Context managers defined in this way can take arguments.

@asynccontextmanager
async def connection(self, timeout_s: float = 5.000):
    await self.connect(timeout_s)
    try:
        yield self
    except Exception as e:
        logger.error(f"Exception in SMPClient:\n{traceback.format_exc()}")
    finally:
        await self.disconnect()

LMK if you're able to test and PR, thanks!

Edit: realizing that this cannot (or should not) be an instance method. Probably doesn't need to even be in the class:

@asynccontextmanager
async def connection(client: SMPClient, timeout_s: float = 5.000) -> AsyncGenerator[SMPClient]:
    await client.connect(timeout_s)
    try:
        yield client
    except Exception as e:
        logger.error(f"Exception in SMPClient:\n{traceback.format_exc()}")
    finally:
        await client.disconnect()

[gudnimg]

Thanks for the reply @JPHutchins ! @asynccontextmanager is new to me, I’ll give it a try.

Decoupling SMPBLETransport and SMPClient sounds reasonable to me.

[gudnimg]

@JPHutchins Hi again 👋 , I took a look at this yesterday and can confirm this works well, this is all called inside a large main function where I have some custom argparse arguments like name and address.

import asyncio
from contextlib import asynccontextmanager
from collections.abc import AsyncGenerator

@asynccontextmanager
async def connection(client: SMPClient, timeout_s: float = 5.0) -> AsyncGenerator[SMPClient]:
    await client.connect(timeout_s)
    try:
        yield client
    except Exception as e:
        print(e)
    finally:
        await client.disconnect()


client = SMPClient(SMPBLETransport(), args.address or args.name)
async with connection(client, timeout_s = 30.0):
    # Performing FOTA over BLE

This @asynccontextmanager thing is pretty cool :)

I'm not sure what's the best approach for a PR if you're planning on changing the classes anyway (decouple them). But if I would make a PR, would it make senmse in this case with @asynccontextmanager to also define it as a static method (@staticmethod) inside the class? That way the function doesn't belong to the class instance.

[JPHutchins]

@JPHutchins Hi again 👋 , I took a look at this yesterday and can confirm this works well, this is all called inside a large main function where I have some custom argparse arguments like name and address.

import asyncio
from contextlib import asynccontextmanager
from collections.abc import AsyncGenerator

@asynccontextmanager
async def connection(client: SMPClient, timeout_s: float = 5.0) -> AsyncGenerator[SMPClient]:
await client.connect(timeout_s)
try:
yield client
except Exception as e:
print(e)
finally:
await client.disconnect()

client = SMPClient(SMPBLETransport(), args.address or args.name)
async with connection(client, timeout_s = 30.0):
# Performing FOTA over BLE
This @asynccontextmanager thing is pretty cool :)

I'm not sure what's the best approach for a PR if you're planning on changing the classes anyway (decouple them). But if I would make a PR, would it make senmse in this case with @asynccontextmanager to also define it as a static method (@staticmethod) inside the class? That way the function doesn't belong to the class instance.

Thanks for testing! I wouldn't wait for that rework 🤣!

I think that defining it in a static method or function outside the class makes sense. Putting it in a static method is more self-documenting as far as purpose, but also more verbose, and I don't have a strong opinion especially if the doc string contains a code block usage example.

async with SMPClient.connection(SMPClient(SMPBLETransport(), args.address or args.name):
    ...