Skip to content

Conversation

@screamerbg
Copy link
Contributor

@screamerbg screamerbg commented Nov 20, 2018

This minimalistic PR adds support to dynamically sett/adjust a test timeout via DUT messages {{__timeout_set,int}}, {{__timeout_adjust,int}}.

The is particularly useful when a device is making a download (say firmware update), where the speed might dynamically change based on various conditions. With this feature, the DUT might notify the host that it's alive and running, just slower than expected.

A typical usecase would be a unified IP connectivity test, where the time to complete over Ethernet and Wifi would be significantly faster than 2G/3G, let alone NBIoT/CAT-M1.

As discussed with @bridadan

@coveralls
Copy link

Coverage Status

Coverage remained the same at 42.424% when pulling 056b2fd on screamerbg:feature_dynamic_timeout into 1c570ec on ARMmbed:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 42.424% when pulling 056b2fd on screamerbg:feature_dynamic_timeout into 1c570ec on ARMmbed:master.

@coveralls
Copy link

coveralls commented Nov 20, 2018

Coverage Status

Coverage remained the same at 42.424% when pulling 5090b13 on screamerbg:feature_dynamic_timeout into 1c570ec on ARMmbed:master.

@theotherjimmy
Copy link
Contributor

@screamerbg Thanks for the PR. Do you need this in a release of htrun soon?

@screamerbg
Copy link
Contributor Author

@theotherjimmy Yes please (if possible).

Here's an example usage - PelionIoT/simple-mbed-cloud-client@6cb6566#diff-82f388e30987a4563c237f17ccb185dbR66

Copy link
Contributor

@bridadan bridadan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Just a few suggestions regarding the keys.

self.logger.prn_inf("%s received"% (key))
callbacks__exit_event_queue = True
break
elif key == '__timeout_set':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The beginning of the test (the "preamble" as its referred to at the top of this file) sets the timeout of the test with the __timeout key. I'd say go ahead and reuse that key instead of making a new one, unless you have any objections.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The __timeout key at "preamble" stage also sets the global timer to 0 and timeout duration. The behavior of __timer_set only sets the timeout duration, but doesn't change the global timer (e.g. time elapsed). I'd prefer not to confuse these two different behaviors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a very good point, thanks very much for clarifying 😄

# Dynamic timeout set
timeout_duration = int(value) # New timeout
self.logger.prn_inf("setting timeout to: %d sec"% int(value))
elif key == '__timeout_adj':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to stay away from abbreviations if the full word is reasonably small. In this case __timeout_adjust is just three more characters. Would you mind "adjusting" (heh, couldn't resist 😄) this key?

Copy link
Contributor Author

@screamerbg screamerbg Dec 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I'll "adj" it! 😄

self.logger.prn_inf("%s received"% (key))
callbacks__exit_event_queue = True
break
elif key == '__timeout_set':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a very good point, thanks very much for clarifying 😄

@screamerbg
Copy link
Contributor Author

Bump @cmonr @theotherjimmy

Copy link
Contributor

@cmonr cmonr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

self.logger.prn_inf("setting timeout to: %d sec"% int(value))
elif key == '__timeout_adjust':
# Dynamic timeout adjust
timeout_duration = timeout_duration + int(value) # adjust time
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was about to ask "what's the difference between adjust and set, but this was simple enough :)

@bridadan bridadan merged commit 53f0aca into ARMmbed:master Dec 12, 2018
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

Successfully merging this pull request may close these issues.

5 participants