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

Change a UART instance's timeout #3434

Open
fragmuffin opened this issue Nov 13, 2017 · 12 comments
Open

Change a UART instance's timeout #3434

fragmuffin opened this issue Nov 13, 2017 · 12 comments

Comments

@fragmuffin
Copy link
Contributor

Changing a UART's timeout behaviour would be fantastic.

>>> import pyb, time
>>> # initialise uart with timeout of 200ms
>>> uart = pyb.UART(1, baudrate=9600, timeout=100)

>>> # test it out.
>>> x =  (time.ticks_ms(), uart.readline(), time.ticks_ms())
>>> x[2] - x[0]
100

However, changing the timeout isn't supported (yet 😉)

>>> uart.timeout = 0
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'UART' object has no attribute 'timeout'
@peterhinch
Copy link
Contributor

I think you need to call uart.init() again with the new timeout value. This type of question is best posted in the forum.

@fragmuffin
Copy link
Contributor Author

Thank you @peterhinch.

I must admit, I didn't pose this as a question because I think it's more intuitive to be able to change the timeout as I've described

There are a few reasons for this:

  • It's not clear what effect init() has, for example, does init() cause any bus downtime?
  • init() has parameter defaults, so uart.init(timeout=0) may (and will probably) change the bus packet format, or baudrate.

So it seems this is an improvement suggestion, not a bug, but still a legitimate issue.

@dpgeorge
Copy link
Member

dpgeorge commented Dec 4, 2018

An alternative to having a separate timeout method/attribute is to specify that the init() method only changes the parameters that are passed in, and the rest are left as-is.

@fragmuffin
Copy link
Contributor Author

@dpgeorge I agree that's an alternative, but it doesn't solve the ambiguity of user code.
Can we get the best of both worlds with something like:

class UART:
    @property
    def timeout(self):
        return self._timeout

    @timeout.setter
    def timeout(self, value):
        self.init(timeout=value)

@dpgeorge
Copy link
Member

dpgeorge commented Dec 4, 2018

but it doesn't solve the ambiguity of user code.

What do you mean by this? Can you give an example of such code?

Can we get the best of both worlds with something like:

The thing with properties is that they are more expensive (in code space) than adding keyword arguments to a method like init(). Furthermore, the API for other peripherals is designed without the use of properties, so it would be inconsistent to add one here. Instead, if it were to be a separate method, it could be uart.timeout([value]) which is a method to get/set the timeout. But then, for consistency, we'd also want to add uart.baudrate([value]), uart.parity([value]), etc. To keep things minimal the init()` method handles everything.

Alternatively, since a UART is like a stream, it could make sense to add uart.setblocking() and/or uart.settimeout() to mimic sockets.

@fragmuffin
Copy link
Contributor Author

What do you mean by this? Can you give an example of such code?

I just mean that, for someone not familiar with this discussion, the following:

uart.timeout = 0.2
uart.init(timeout=0.2)

may be functionally different:

  • the first seems to dynamically change the timeout, while keeping everything else about the uart instance the same, and alive (still receiving)
  • the second looks like it may re-initialize the uart, which may involve de-initializing it to start with. Without looking further into the documentation, a reset to default baud-rates and, and some downtime would not be a surprise.

I understand that the 2 lines could be functionally the same, but it's not clear until you read the documentation.

Instead, if it were to be a separate method, it could be uart.timeout([value])

Yep! That's a big 👍 from me.
Personally I don't mind the uart.timeout():uart.timeout(x) as a get:set.
The @properties being expensive isn't surprising (I must admit)

@peterhinch
Copy link
Contributor

Alternatively, since a UART is like a stream, it could make sense to add uart.setblocking() and/or uart.settimeout() to mimic sockets.

That would be my choice.

@pfalcon
Copy link
Contributor

pfalcon commented Dec 4, 2018

The @properties being expensive isn't surprising (I must admit)

It may be less obvious, but having extra methods is also expensive - each method adds quite a bunch of boilerplate beyond the code which actually does something useful. So, reading docs/adding code comments is imperative.

@robert-hh
Copy link
Contributor

robert-hh commented Dec 4, 2018

An alternative to having a separate timeout method/attribute is to specify that the init() method only changes the parameters that are passed in, and the rest are left as-is.

At first I thought that would be nice, but then there would be not default parameters any more in the init() call, or the instantiation of a uart object. So keeping it like it is may be the better option.

@dhylands
Copy link
Contributor

dhylands commented Dec 4, 2018

Another approach would be to have 2 functions. One like .init() which assumes that it's initializing everything and arguments have default values which are reasonable. The second would be something like .set_parameter() and would take all of the same keyword arguments as init but it would only act on the ones which were actually provided. .init() could call .set_parameter.

This still doesn't cover retrieving values, but I thought I'd throw it out as an idea.

@dhalbert
Copy link

dhalbert commented Dec 4, 2018

  1. We do something like the init() thing in some CircuitPython APIs. For instance busio.SPI() takes some parameters in the constructor, but then there's also an SPI.configure() method that takes keyword arguments for parameters that can be changed after construction, often in concert (it doesn't make sense to change one at a time). Maybe configure is an appropriate name for you here.

We also use properties a lot more freely, though we don't provide access to everything that way. (We trade being Pythonic against performance; I understand why you've gone the other way.)

  1. For UART, we have been providing functionality that's congruent with PySerial, partly because its API has evolved to cover a bunch of practical use cases, and also because for Adafruit's wrapper library that allows our device libraries to run on RPi and the like, we use PySerial underneath to substitute for the native UART implementation.

@pfalcon
Copy link
Contributor

pfalcon commented Dec 4, 2018

At first I thought that would be nice, but then there would be not default parameters any more in the init() call, or the instantiation of a uart object.

And indeed, listing specific default param values is the root of problem. Instead of being documented as

UART.init(baudrate=9600, bits=8, parity=None, stop=1, *, ...)

as

UART.init(baudrate=None, bits=None, parity=None, stop=None, *, ...)

That would make it clear what's meant by the calls .init(timeout=10) and .init(None, 7). Except that there's a problem with parity, which now needs to be -1 for no parity.

something like .set_parameter()

MicroPython already has solution for setting and querying arbitrary params on an object: http://docs.micropython.org/en/latest/library/network.html?highlight=network#network.AbstractNIC.config , readily applicable to machine module either. Indeed, one can argue that .init() should have been called .config() from the beginning. Now it can be just alias to .init(). (As long as .init() doesn't take str as a 1st positional param).

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

No branches or pull requests

7 participants