Make pause after sending SDO request configurable

[sveinse]

This is a minor fix that introduces a configurable class variable to set the delay after sending an SDO request. This is instead of having a hardcoded value.

The same should probably be done for lss.py, but I don't really have an LSS-compatible CANopen-device, so I cannot verify it.

[acolomb]

@erlend-aasland Is this not sufficient?

https://canopen.readthedocs.io/en/latest/sdo.html#canopen.sdo.SdoClient.PAUSE_BEFORE_SEND

[acolomb]

You are in fact changing the default behavior here. What are the consequences? I'd hate breaking people's usage of the library because they are not aware of this change. There must be a reason why this delay was added. Although I completely agree it should be configurable.

[sveinse]

That was actually unintentional. I meant it to initalize PAUSE_AFTER_SEND = 0.1 to not modify the behavior. I will update the PR.

Edit:. Fixed.

[erlend-aasland]

@erlend-aasland Is this not sufficient?

https://canopen.readthedocs.io/en/latest/sdo.html#canopen.sdo.SdoClient.PAUSE_BEFORE_SEND

Yes. I see now that you're using .. autoclass.

[erlend-aasland]

Doc nit:

Suggested change

	#: Seconds to wait after sending a request
	#: Seconds to wait after sending a request.

[acolomb]

Please not. It's inconsistent with the other docs and just one more useless character in the source. Let's not get into such nit-picks :-)

[erlend-aasland]

There are docstrings both with and without dots; I'm not sure if there is already consistency ;) Nits are nits; it is ok to ignore them :)

[erlend-aasland]

Let me clarify: you are right that the docstrings are consistent, but the rendered docs are not consistent, since the docs for attributes and methods include correct punctuation. Up to you to if this needs cleaning up ;)

[acolomb]

I think it doesn't need any action on our part. If the Sphinx automatically appends a period, that's fine and expected. Doxygen does likewise IIRC. Some Python style checkers (I'm using flake8) already point out inconsistencies in docstrings. Such as "missing period", "not a single sentence" or "not in imperative mood". There are no analogous conventions for attribute descriptions AFAIK, but it does make sense to not document them with an imperative sentence. Thus also no full-stop required. It's just a descriptive phrase.

[christiansandberg]

Maybe it wasn't clear but it is only used for retries, so maybe it should have been named RETRY_DELAY or something.

[acolomb]

Well, we can change the name easily as long as it's not released yet. Sorry for the not looking closer at the context.

[sveinse]

Technically if a retry occurs, the delay will be RETRY_DELAY + PAUSE_BEFORE_SEND due to the loop. Before I go ahead and simply rename it into RETRY_DELAY, do we want to do something else with the logic while at it for consistency? What do you think of the proposal below?

def send_request(self, request):
    retries_left = self.MAX_RETRIES
    if self.PAUSE_BEFORE_SEND:                # <-- Moved
        time.sleep(self.PAUSE_BEFORE_SEND)    # <-- Moved
    while True:
        try:
            self.network.send_message(self.rx_cobid, request)
        except CanError as e:
            # Could be a buffer overflow. Wait some time before trying again
            retries_left -= 1
            if not retries_left:
                raise
            logger.info(str(e))
            if self.RETRY_DELAY:               # <-- Renamed
                time.sleep(self.RETRY_DELAY)   # <-- Renamed
        else:
            break

[sveinse]

I've created a new branch for this I can add to a new PR if this is what we want: master...sveinse:canopen-asyncio:feature-sdo-pause-pt2

[christiansandberg]

Yes that's probably how it should be. If you could make a PR of that branch that would be appreciated!