[microTVM][Zephyr] Fix: Test fails on hardware because of short timeout

[mehrdadh]

Currently test_zephyr_aot:test_tflite fails on stm32l4r5zi_nucleo board because of short timeout. This PR adds option to pass timeout to _get_message function and we set it to be 60 seconds.

cc @areusch @gromero

[gromero]

@mehrdadh Hi! The PR title says the test fails because of short timeout but your fix is reducing it even further afaics?

Also I miss a bit a better commit message explaining more about the "rational/background" for changes in question do. Of course, they are simple in essence but for instance, the commit "71dfd4b" only mentions a "fix" in the title. It would be good to at least mention the occasion when it happens (which hardware, if it always happen or not, etc) for the future when people would be looking at the commit via git log. Thumbs up for using a patchset for the two changes (adding the parameter plus setting the proper timeout that worked for you).

I think that a better elaboration of the commit messages in general will specially help others which are/will catch up with the TVM project.

[mehrdadh]

@gromero thanks for your feedback. I changed the PR description with more details.
Before the timeout was manually set inside the read_line function and it was 10sec. Now, I changed it to 60s.
I changed the commit message as well.

[gromero]

@gromero thanks for your feedback. I changed the PR description with more details.
Before the timeout was manually set inside the read_line function and it was 10sec. Now, I changed it to 60s.
I changed the commit message as well.

I see now. You're changing values 5s (for writing) and 10s (for reading) for the transport both to 70s right? (You said 60s above but I see timeout_sec = 70 - is there a catch in the code here or it was just a typo in your comment above?).

So what confused me is that in your first comment in the first commit of the patchset where you add the new parameter timeout_sec you actually increase the timeout value for both read and write to 100 seconds and then, on the second commit, you decrease the timeout_sec value to 70 seconds, so when both commits are squashed (pity Github squashes it before merging... but that's another story) you're right, it will amount to a increase in the timeout to 70s (for both read and write). So, if I'm following it correctly, the suggestion I have is to split better next time the patchset, like not saying a commit adds a parameter and the code adds a parameters plus does other changes like modifying other values (like the timeout_sec for read and write). That eases a lot the review process, specially for more complex changes.

Your second commit message version is better but I think you could s/change/Increase/ and s/a reasonable value/70 seconds/ to inform to the readers more precisely what that commit does when applied (maybe next time, so feel free to change at will or only when you get more reviews). Anyways, just as an example, I would split them as:

1. gromero@8a6277d - Add 'timeout_sec' parameter
2. gromero@532fe2b - Increase test timeout to 70 sec

Finally, about the code itself, just out of curiosity, I'm wondering if 70 seconds isn't too much for a device to reply in this case. In your experiments have you tried any smaller value?

[jroesch]

@mehrdadh just double check to make sure @gromero is good with the change and then feel free to merge yourself

[gromero]

LGTM. Thanks.