Timespec refactor (again) #45

mikepurvis · 2013-10-29T04:02:01Z

A simpler, gentler take on what was previously discussed in #42 and #43.

Everything is handled in ms, with a timespec_from_ms function providing last-minute conversion to timespecs for use in pselect.
A MillisecondTimer class is provided which handles the total_timeout functionality in read and write. Its set/remaining is in millis, but internally it stores the expiry point as a timespec for maximal resolution.
No funky math or operations on timespecs.

mikepurvis · 2013-10-29T04:04:44Z

src/impl/unix.cc

@@ -551,41 +542,25 @@
  }
  fd_set writefds;
  size_t bytes_written = 0;
-  struct timeval timeout;
-  timeout.tv_sec =                    timeout_.write_timeout_constant / 1000;
-  timeout.tv_usec = static_cast<int> (timeout_.write_timeout_multiplier % 1000);


I don't understand what this logic is. My implementation below attempts to do what I believe is correct behaviour rather than follow this.

timeout_.write_timeout_multiplier % 1000 is the remainder of milliseconds which is left over from removing the seconds. Then the next line multiplies it by 1000 to get it into microseconds.

If that's the case, shouldn't it be write_timeout_constant in both lines, rather than write_timeout_multiplier in the second?

AFAICT, it should be write_timeout_constant + (write_timeout_multiplier * expected_bytes), with that resulting milliseconds value then converted to timeval via the div/mod procedure. In any case, that's what the proposed replacement logic does.

Yes, that was likely a typo...

wjwwood · 2013-10-31T01:48:52Z

Sorry this is taking so long for me to review, I have been getting distracted, and I want to make a sold few passes over this since it is such a substantial change.

Hopefully I can complete my feedback tonight sometime.

mikepurvis · 2013-10-31T02:10:12Z

No worries; I appreciate the thoroughness!

mikepurvis · 2013-10-31T04:41:31Z

Okay, I've remove the modulus operator and the exception logic.

EDIT: Looks like that timeout logic isn't quite right yet—I'm seeing some weird negative timespec issues.

mikepurvis · 2013-10-31T05:14:39Z

That should do it.

wjwwood · 2013-10-31T21:50:39Z

Looks good to me, I opened a pull request against your pull request branch to use static_cast rather than C-style casts.

clearpathrobotics#3

wjwwood · 2013-10-31T23:14:02Z

Looks like this pull request needs a rebase.

…reater clarity.

C-style casting can result in undesired reinterpret_casts So we should avoid them, see: http://stackoverflow.com/questions/332030/when-should-static-cast-dynamic-cast-and-reinterpret-cast-be-used

mikepurvis · 2013-10-31T23:48:19Z

Done.

wjwwood · 2013-11-01T00:04:25Z

Thanks @mikepurvis. Ok, this looks ready to me. I want to get some more eyes on it though.

@ashgti @dirk-thomas @davidhodo

Can I get some code review?

Here is a brief summary of this change set:

consolidated timeout related math in a new MillisecondTimer class
switch to using pselect in read and write
refactor read and write's timeout logic
- using the MillisecondTimer class
- removing conditional logic as pselect doesn't modify the timespec struct for you

dirk-thomas · 2013-11-01T00:30:51Z

src/impl/unix.cc

+  int64_t tv_nsec = expiry.tv_nsec + (millis * 1e6);
+  if (tv_nsec > 1e9) {
+    int64_t sec_diff = tv_nsec / static_cast<int> (1e6);
+    expiry.tv_nsec = tv_nsec - static_cast<int> (1e6 * sec_diff);


The condition with 1e9 but the math operations with 1e6 look bogus to me.

I agree with @dirk-thomas I think those should be 1e9 because we are going from nanoseconds to seconds there.

Whoops—and it was the wrong number even in the original modulo logic. Fixed now.

mikepurvis · 2013-11-14T19:35:45Z

Just to check back in here, I've been using this code consistently for the past week with device drivers on Grizzly, and it seems to be working as expected—I'm not seriously exercising the timing/timeout logic, but given the bugs in the present implementation, I'm not sure anyone else is either.

The real solution to verifying it is #47, but I'm hoping we can merge this in advance of that being done.

wjwwood · 2013-11-14T22:14:24Z

@mikepurvis Sorry about that, I have been busy, I'll revisit this asap.

wjwwood · 2013-11-14T22:38:42Z

Regardless of comprehensive tests and portable tests for the actual end-to-end serial behavior, I think the MillisecondTimer class is easily unit tested in a portable fashion. I would like to see some basic unit tests for this class, as they would have likely caught both @dirk-thomas's comments as well as my most recent one about the conditional logic.

mikepurvis · 2013-11-15T00:01:02Z

Sure. Part of the idea of creating the class was to make a testable unit. Where would you like the tests to be, and how should their building and execution be handled?

wjwwood · 2013-11-15T00:02:52Z

I would put them in the test folder, maybe in a unit subfolder. You can use gtest and I think catkin_add_gtest to have them built/run.

wjwwood · 2013-11-15T00:03:09Z

Sorry the tests folder.

… test builds into separate makefile.

mikepurvis · 2013-11-19T03:11:34Z

Alright, have added some simple tests for MillisecondTimer—unfortunately, not perfectly deterministic, so there will be spurious failures. Ideal would be to somehow bubble up more information about the failed tests, to see if they represent actual corner cases or just flukes.

Have also adding test running to travis, with the proviso that test failures do not fail the build. I can't see an easy way to do this, as it appears that the catkin test make targets return zero pretty much no matter what. I believe you'd need to throw in a Perl or Python one-liner to actually check the result XMLs for failure elements.

wjwwood · 2013-11-20T21:24:28Z

Looks good to me.

Timespec refactor (again)

mikepurvis · 2013-11-21T03:55:42Z

Thanks for the review and merge!

wjwwood · 2013-11-21T03:56:35Z

Thanks for the contribution.

mikepurvis reviewed Oct 29, 2013
View reviewed changes

mikepurvis and others added 7 commits October 31, 2013 19:43

Add MillisecondTimer class.

16a0246

Implement MillisecondTimer in the unix read() and write() functions.

6747711

Rename now function to timespec_now and timeout_time to expiry, for g…

d8a1ef4

…reater clarity.

Eliminate modulus operator.

589e7b9

Remove TimerExpiredException.

5820056

Of course, the timespec_from_ms function must return its result.

3f2ed36

use static casts rather than C-style casting

cfac5bb

C-style casting can result in undesired reinterpret_casts So we should avoid them, see: http://stackoverflow.com/questions/332030/when-should-static-cast-dynamic-cast-and-reinterpret-cast-be-used

dirk-thomas reviewed Nov 1, 2013
View reviewed changes

mikepurvis and others added 2 commits October 31, 2013 21:48

Divide by 1e9 to get seconds from nanoseconds, instead of 1e6.

2e5e8f9

Fix for computing an expiry without rollover.

fbffc18

Wrap nanoseconds when >= 1e9, rather than > 1e9.

c3855ad

Pre-fill buffer at start of read, to avoid the select if unnecessary.

a9bf8d8

mikepurvis mentioned this pull request Nov 18, 2013

Buffer fillup issue with 20+ hz data stream #51

Closed

mikepurvis added 2 commits November 18, 2013 21:35

Add unix timer tests, clarify tests are only on unix at present, move…

9c432f9

… test builds into separate makefile.

Build tests on Travis.

a633418

wjwwood added a commit that referenced this pull request Nov 20, 2013

Merge pull request #45 from clearpathrobotics/timespec-refactor2

f051c0a

Timespec refactor (again)

wjwwood merged commit f051c0a into wjwwood:master Nov 20, 2013

mikepurvis deleted the timespec-refactor2 branch November 21, 2013 03:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Timespec refactor (again) #45

Timespec refactor (again) #45

mikepurvis commented Oct 29, 2013

mikepurvis Oct 29, 2013

wjwwood Oct 30, 2013

mikepurvis Oct 30, 2013

wjwwood Oct 30, 2013

wjwwood commented Oct 31, 2013

mikepurvis commented Oct 31, 2013

mikepurvis commented Oct 31, 2013

mikepurvis commented Oct 31, 2013

wjwwood commented Oct 31, 2013

wjwwood commented Oct 31, 2013

mikepurvis commented Oct 31, 2013

wjwwood commented Nov 1, 2013

dirk-thomas Nov 1, 2013

wjwwood Nov 1, 2013

mikepurvis Nov 1, 2013

mikepurvis commented Nov 14, 2013

wjwwood commented Nov 14, 2013

wjwwood commented Nov 14, 2013

mikepurvis commented Nov 15, 2013

wjwwood commented Nov 15, 2013

wjwwood commented Nov 15, 2013

mikepurvis commented Nov 19, 2013

wjwwood commented Nov 20, 2013

mikepurvis commented Nov 21, 2013

wjwwood commented Nov 21, 2013

Timespec refactor (again) #45

Timespec refactor (again) #45

Conversation

mikepurvis commented Oct 29, 2013

mikepurvis Oct 29, 2013

Choose a reason for hiding this comment

wjwwood Oct 30, 2013

Choose a reason for hiding this comment

mikepurvis Oct 30, 2013

Choose a reason for hiding this comment

wjwwood Oct 30, 2013

Choose a reason for hiding this comment

wjwwood commented Oct 31, 2013

mikepurvis commented Oct 31, 2013

mikepurvis commented Oct 31, 2013

mikepurvis commented Oct 31, 2013

wjwwood commented Oct 31, 2013

wjwwood commented Oct 31, 2013

mikepurvis commented Oct 31, 2013

wjwwood commented Nov 1, 2013

dirk-thomas Nov 1, 2013

Choose a reason for hiding this comment

wjwwood Nov 1, 2013

Choose a reason for hiding this comment

mikepurvis Nov 1, 2013

Choose a reason for hiding this comment

mikepurvis commented Nov 14, 2013

wjwwood commented Nov 14, 2013

wjwwood commented Nov 14, 2013

mikepurvis commented Nov 15, 2013

wjwwood commented Nov 15, 2013

wjwwood commented Nov 15, 2013

mikepurvis commented Nov 19, 2013

wjwwood commented Nov 20, 2013

mikepurvis commented Nov 21, 2013

wjwwood commented Nov 21, 2013