Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Timespec refactor (again) #45

Merged
merged 13 commits into from
Nov 20, 2013
Merged

Timespec refactor (again) #45

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
16a0246
Add MillisecondTimer class.
mikepurvis Oct 28, 2013
6747711
Implement MillisecondTimer in the unix read() and write() functions.
mikepurvis Oct 29, 2013
d8a1ef4
Rename now function to timespec_now and timeout_time to expiry, for g…
mikepurvis Oct 29, 2013
589e7b9
Eliminate modulus operator.
mikepurvis Oct 31, 2013
5820056
Remove TimerExpiredException.
mikepurvis Oct 31, 2013
3f2ed36
Of course, the timespec_from_ms function must return its result.
mikepurvis Oct 31, 2013
cfac5bb
use static casts rather than C-style casting
wjwwood Oct 31, 2013
2e5e8f9
Divide by 1e9 to get seconds from nanoseconds, instead of 1e6.
mikepurvis Nov 1, 2013
fbffc18
Fix for computing an expiry without rollover.
mikepurvis Nov 8, 2013
c3855ad
Wrap nanoseconds when >= 1e9, rather than > 1e9.
mikepurvis Nov 15, 2013
a9bf8d8
Pre-fill buffer at start of read, to avoid the select if unnecessary.
mikepurvis Nov 18, 2013
9c432f9
Add unix timer tests, clarify tests are only on unix at present, move…
mikepurvis Nov 19, 2013
a633418
Build tests on Travis.
mikepurvis Nov 19, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions include/serial/impl/unix.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,16 @@ using std::invalid_argument;
using serial::SerialException;
using serial::IOException;

class MillisecondTimer {
public:
MillisecondTimer(const uint32_t millis);
int64_t remaining();

private:
static timespec timespec_now();
timespec expiry;
};

class serial::Serial::SerialImpl {
public:
SerialImpl (const string &port,
Expand Down
188 changes: 81 additions & 107 deletions src/impl/unix.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,60 @@
using std::string;
using std::stringstream;
using std::invalid_argument;
using serial::MillisecondTimer;
using serial::Serial;
using serial::SerialException;
using serial::PortNotOpenedException;
using serial::IOException;


MillisecondTimer::MillisecondTimer (const uint32_t millis)
: expiry(timespec_now())
{
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);
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

expiry.tv_sec += sec_diff;
}
}

int64_t
MillisecondTimer::remaining ()
{
timespec now(timespec_now());
int64_t millis = (expiry.tv_sec - now.tv_sec) * 1e3;
millis += (expiry.tv_nsec - now.tv_nsec) / 1e6;
return millis;
}

timespec
MillisecondTimer::timespec_now ()
{
timespec time;
# ifdef __MACH__ // OS X does not have clock_gettime, use clock_get_time
clock_serv_t cclock;
mach_timespec_t mts;
host_get_clock_service(mach_host_self(), CALENDAR_CLOCK, &cclock);
clock_get_time(cclock, &mts);
mach_port_deallocate(mach_task_self(), cclock);
time.tv_sec = mts.tv_sec;
time.tv_nsec = mts.tv_nsec;
# else
clock_gettime(CLOCK_REALTIME, &time);
# endif
return time;
}

timespec
timespec_from_ms (const uint32_t millis)
{
timespec time;
time.tv_sec = millis / 1e3;
time.tv_nsec = (millis - (time.tv_sec * 1e3)) * 1e6;
return time;
}

Serial::SerialImpl::SerialImpl (const string &port, unsigned long baudrate,
bytesize_t bytesize,
parity_t parity, stopbits_t stopbits,
Expand Down Expand Up @@ -253,7 +301,7 @@ Serial::SerialImpl::reconfigurePort ()
// other than those specified by POSIX. The driver for the underlying serial hardware
// ultimately determines which baud rates can be used. This ioctl sets both the input
// and output speed.
speed_t new_baud = static_cast<speed_t>(baudrate_);
speed_t new_baud = static_cast<speed_t> (baudrate_);
if (-1 == ioctl (fd_, IOSSIOSPEED, &new_baud, 1)) {
THROW (IOException, errno);
}
Expand All @@ -266,7 +314,7 @@ Serial::SerialImpl::reconfigurePort ()
}

// set custom divisor
ser.custom_divisor = ser.baud_base / (int) baudrate_;
ser.custom_divisor = ser.baud_base / static_cast<int> (baudrate_);
// update flags
ser.flags &= ~ASYNC_SPD_MASK;
ser.flags |= ASYNC_SPD_CUST;
Expand Down Expand Up @@ -404,35 +452,6 @@ Serial::SerialImpl::available ()
}
}

inline void
get_time_now (struct timespec &time)
{
# ifdef __MACH__ // OS X does not have clock_gettime, use clock_get_time
clock_serv_t cclock;
mach_timespec_t mts;
host_get_clock_service(mach_host_self(), CALENDAR_CLOCK, &cclock);
clock_get_time(cclock, &mts);
mach_port_deallocate(mach_task_self(), cclock);
time.tv_sec = mts.tv_sec;
time.tv_nsec = mts.tv_nsec;
# else
clock_gettime(CLOCK_REALTIME, &time);
# endif
}

inline void
diff_timespec (timespec &start, timespec &end, timespec &result) {
if (start.tv_sec > end.tv_sec) {
throw SerialException ("Timetravel, start time later than end time.");
}
result.tv_sec = end.tv_sec - start.tv_sec;
result.tv_nsec = end.tv_nsec - start.tv_nsec;
if (result.tv_nsec < 0) {
result.tv_nsec = 1e9 - result.tv_nsec;
result.tv_sec -= 1;
}
}

size_t
Serial::SerialImpl::read (uint8_t *buf, size_t size)
{
Expand All @@ -442,58 +461,29 @@ Serial::SerialImpl::read (uint8_t *buf, size_t size)
}
fd_set readfds;
size_t bytes_read = 0;
// Setup the total_timeout timeval
// This timeout is maximum time before a timeout after read is called
struct timeval total_timeout;

// Calculate total timeout in milliseconds t_c + (t_m * N)
long total_timeout_ms = timeout_.read_timeout_constant;
total_timeout_ms += timeout_.read_timeout_multiplier*static_cast<long>(size);
total_timeout.tv_sec = total_timeout_ms / 1000;
total_timeout.tv_usec = static_cast<int>(total_timeout_ms % 1000);
total_timeout.tv_usec *= 1000; // To convert to micro seconds
// Setup the inter byte timeout
struct timeval inter_byte_timeout;
inter_byte_timeout.tv_sec = timeout_.inter_byte_timeout / 1000;
inter_byte_timeout.tv_usec =
static_cast<int> (timeout_.inter_byte_timeout % 1000);
inter_byte_timeout.tv_usec *= 1000; // To convert to micro seconds
total_timeout_ms += timeout_.read_timeout_multiplier * static_cast<long> (size);
MillisecondTimer total_timeout(total_timeout_ms);

while (bytes_read < size) {
// Setup the select timeout timeval
struct timeval timeout;
// If the total_timeout is less than the inter_byte_timeout
if (total_timeout.tv_sec < inter_byte_timeout.tv_sec
|| (total_timeout.tv_sec == inter_byte_timeout.tv_sec
&& total_timeout.tv_usec < inter_byte_timeout.tv_sec))
{
// Then set the select timeout to use the total time
timeout = total_timeout;
} else {
// Else set the select timeout to use the inter byte time
timeout = inter_byte_timeout;
int64_t timeout_remaining_ms = total_timeout.remaining();
if (timeout_remaining_ms <= 0) {
// Timed out
break;
}

// Timeout for the next select is whichever is less of the remaining
// total read timeout and the inter-byte timeout.
timespec timeout(timespec_from_ms(std::min(static_cast<uint32_t> (timeout_remaining_ms),
timeout_.inter_byte_timeout)));

FD_ZERO (&readfds);
FD_SET (fd_, &readfds);
// Begin timing select
struct timespec start, end;
get_time_now (start);

// Call select to block for serial data or a timeout
int r = select (fd_ + 1, &readfds, NULL, NULL, &timeout);
// Calculate difference and update the structure
get_time_now (end);
// Calculate the time select took
struct timespec diff;
diff_timespec (start, end, diff);
// Update the timeout
if (total_timeout.tv_sec <= diff.tv_sec) {
total_timeout.tv_sec = 0;
} else {
total_timeout.tv_sec -= diff.tv_sec;
}
if (total_timeout.tv_usec <= (diff.tv_nsec / 1000)) {
total_timeout.tv_usec = 0;
} else {
total_timeout.tv_usec -= (diff.tv_nsec / 1000);
}
int r = pselect (fd_ + 1, &readfds, NULL, NULL, &timeout, NULL);

// Figure out what happened by looking at select's response 'r'
/** Error **/
Expand Down Expand Up @@ -559,41 +549,25 @@ Serial::SerialImpl::write (const uint8_t *data, size_t length)
}
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);
timeout.tv_usec *= 1000; // To convert to micro seconds

// Calculate total timeout in milliseconds t_c + (t_m * N)
long total_timeout_ms = timeout_.write_timeout_constant;
total_timeout_ms += timeout_.write_timeout_multiplier * static_cast<long> (length);
MillisecondTimer total_timeout(total_timeout_ms);

while (bytes_written < length) {
int64_t timeout_remaining_ms = total_timeout.remaining();
if (timeout_remaining_ms <= 0) {
// Timed out
break;
}
timespec timeout(timespec_from_ms(timeout_remaining_ms));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, that was likely a typo...


FD_ZERO (&writefds);
FD_SET (fd_, &writefds);
// On Linux the timeout struct is updated by select to contain the time
// left on the timeout to make looping easier, but on other platforms this
// does not occur.
#if !defined(__linux__)
// Begin timing select
struct timespec start, end;
get_time_now(start);
#endif

// Do the select
int r = select (fd_ + 1, NULL, &writefds, NULL, &timeout);
#if !defined(__linux__)
// Calculate difference and update the structure
get_time_now(end);
// Calculate the time select took
struct timespec diff;
diff_timespec(start, end, diff);
// Update the timeout
if (timeout.tv_sec <= diff.tv_sec) {
timeout.tv_sec = 0;
} else {
timeout.tv_sec -= diff.tv_sec;
}
if (timeout.tv_usec <= (diff.tv_nsec / 1000)) {
timeout.tv_usec = 0;
} else {
timeout.tv_usec -= (diff.tv_nsec / 1000);
}
#endif
int r = pselect (fd_ + 1, NULL, &writefds, NULL, &timeout, NULL);

// Figure out what happened by looking at select's response 'r'
/** Error **/
Expand Down