ros / roscpp_core

ros distribution sandbox
89 stars 116 forks source link

rostime: Added useful time and duration constants. #123

Closed peci1 closed 1 year ago

peci1 commented 4 years ago

For Time, WallTime and SteadyTime, I added static constants MIN, MAX, ZERO and UNINITIALIZED.

For Duration and WallDuration, I added static constants MIN, MAX, ZERO, NANOSECOND, MICROSECOND, MILLISECOND, SECOND, MINUTE, HOUR, DAY, WEEK and YEAR.

In the header files, I added explicit specialization declarations for each of the constants. The build and tests succeeded even without these definitions, but clang suggested to add them and I did so.

I'm not sure if YEAR should represent 365 days or 365.2425. I vouch for the latter as it yields smaller errors if you count with large durations (e.g. in 10 years, the 365-day year gives you 2 day error, whereas 365.2425 day has error somewhere around half a day).

These constants are mainly syntactic sugar, but I imagine they could become used over time when people know about them.

peci1 commented 4 years ago

The tests are kind of just repeating the definitions, but I couldn't think of anything better...

dirk-thomas commented 4 years ago

I'm not sure if YEAR should represent 365 days or 365.2425. I vouch for the latter as it yields smaller errors if you count with large durations (e.g. in 10 years, the 365-day year gives you 2 day error, whereas 365.2425 day has error somewhere around half a day).

Because of this uncertainty - the duration class is simply not designed to handle things like leap years - I would rather not provide a constant for it. Either value will surprise a non-significant amount of users.

peci1 commented 4 years ago

Because of this uncertainty - the duration class is simply not designed to handle things like leap years - I would rather not provide a constant for it.

Good point, I'll remove the constant for year

peci1 commented 2 years ago

Wow, this PR has somehow slipped through my fingers... But I found it again! I removed the constant for YEAR due to the mentioned ambiguity. I also removed WEEK as it doesn't seem very useful to me.

@dirk-thomas Could you please re-review?

peci1 commented 1 year ago

@dirk-thomas friendly ping

dirk-thomas commented 1 year ago

@peci1 Sorry, I am not involved in the project anymore for more than 2.5 years.

peci1 commented 1 year ago

In that case, this needs to be updated:

https://github.com/ros/roscpp_core/blob/72ce04f8b2849e0e4587ea4d598be6ec5d24d8ca/rostime/package.xml#L7

(and probably others)

Do you know who the maintainer is?

dirk-thomas commented 1 year ago

Unfortunately I don't know. Since the git history doesn't show any new commits, maybe ask on discourse?

peci1 commented 1 year ago

@ros-pull-request-builder retest this please

peci1 commented 1 year ago

Merging without other approvals since I'm the only maintainer.

peci1 commented 1 year ago

@Mergifyio backport melodic-devel

mergify[bot] commented 1 year ago

backport melodic-devel

✅ Backports have been created

* [#139 rostime: Added useful time and duration constants. (backport #123)](https://github.com/ros/roscpp_core/pull/139) has been created for branch `melodic-devel`