refactor: use a single configurable timeout value

[etiennedm]

This is adressing intercreate/smpmgr#71. Replace magic default values to use a single per-client default timeout value. This allows higher layers (e.g. smpmgr) to specify a timeout value when necessary and otherwise rely on the default set when creating the client.

I removed a bunch of default values that were all over the place, please comment on the need to have these, I might be missing something!

[JPHutchins]

Thank you! Specific to the upload / upgrade scenario, I think that we should expose a configurable "first timeout" with a reasonably long value. On some SMP servers, the first upload request may take a long time, especially on external flash. Generally, the short timeout is good because we want to detect an erroring SMP server, but we have to be able to handle a scenario where the request takes on the order of 40 seconds.

Smpmgr could expose this only where relevant, as an option on the upgrade and upload commands, rather than yet another global transport option.

[etiennedm]

Yes that makes sense. See the updated version (mostly a revert).

How about taking the sum of both first_timeout_s and subsequent_timeout_s for the first request timeout? With the given scenario, it would make sense. For instance an openthread node which is a SED (sleepy end device) and might check for packets every 60s. I would want the subsequent_timeout_s to be 70s (set through the SMPClient _timeout property), and the timeout for the first request to be 70s + 40s = first_timeout_s + subsequent_timeout_s if the first ImageUploadWrite takes a long time to erase the flash.

[JPHutchins]

Yes that makes sense. See the updated version (mostly a revert).

How about taking the sum of both first_timeout_s and subsequent_timeout_s for the first request timeout? With the given scenario, it would make sense. For instance an openthread node which is a SED (sleepy end device) and might check for packets every 60s. I would want the subsequent_timeout_s to be 70s (set through the SMPClient _timeout property), and the timeout for the first request to be 70s + 40s = first_timeout_s + subsequent_timeout_s if the first ImageUploadWrite takes a long time to erase the flash.

I read your changes more carefully and see that you did leave this exposed, so I think it's OK as is. I agree with your assessment about guessing at a reasonable value by adding them together, but it sounds hard to document and keep track of!

So, in your example, user would need to call upload with first_timeout_s=110, subsequent_timeout_s=70. Or, simpler, leave subsequent_timeout_s as None and use the value given in the constructor.

[JPHutchins]

This is a really great improvement, thank you! I'm mostly suggesting adding the _s seconds unit for consistency. Functionally, this is a great upgrade!

We do have a discussion about improving the architecture. Presently, "transport" and "client" are sorta intertwined in ways that prevent some nice context management or more flexible usage in general - including the "global transport options" issue faced by smpmgr. Would love your feedback if you have time: #58

I bring this up because ownership of timeout raises questions about the transport/client relationship - is the timeout parameter of the transport (SED example you gave) or the protocol (the initial upload request taking longer, maybe)?