Fix time conversion to ms

Signed-off-by: Andreas Rauschert andreas.rb.rauschert@bmw.de

requested review from @jtschea and @ckochendoerfer

I am a bit confused on why this is necessary. Since Time is units::time::millisecond_t, I would have expected that "units::time::second(duration)" is implicitly converted to milliseconds?

Yes, it does. But the current implementation breaks our backward compatibility. We have a resolution of whole ms and the new conversion via chrono::milliseconds guarantees this resolution.

Ok, thanks for clarifying. Our use case also requires milliseconds as integers.

Maybe we could define Time as: using Time = units::unit_t<units::time::millisecond, int, units::linear_scale>

I have not tried using int types before, so I am not sure if this would work. But could be worth trying out.

Good idea! I tested it with int64_t and in general it works. The impacts are:

• the SecondsToTime(double) also needs to be adjusted to first convert to unit::milliseconds_t (double) and then to Time (int), otherwise the ms are cut off during the cast. This means you must not/cannot cast yourself anymore from units::seconds_t to Time
• you cannot do calculations with the default units time types anymore without casting (so e.g. my_mantle_time + 10_ms) doesn't work since the types are different. You have to write (my_mantle_time + mantle_api::Time{10})

What is your preference?

I am still a bit undecieded. With the second point I don't see any issues. The places which currently already require milliseconds as integer need a conversion anyway. In the modules where we do actual calculations they completly run in doubles and could still use the units literals.

The only thing that worries me with the first point is, that users might assume that the conversion from units::seconds_t to Time to works implicitly. Since this doesn't throw any compile errors, those mistakes could be hard to find.

One other idea, is it possible to define units conversions manually? I know you can specify conversion ratios, maybe this can be extended? Mabye we could implement our own conversion from units::time to mantle::Time?

I couldn't find out how to do this. The implicit conversions are defined in units.h in the unit_t class definition. I assume defining a specialization for our mantle::Time would need an invalid redefinition of class units::unit_t inside the mantleAPI code.

I still lack experience with the units extension. To make a final decision it probably requires some more experimenting. In order to move forward, I would be fine with both solutions at the moment.

I would prefer to stick closer to the units, but this could lead to missuse and hardtofind errors at the moment.

Should we discuss it in our meeting tomorrow?

changed this line in version 2 of the diff

Yes, let's discuss. I just made one last change for our backward compatibility and made the secondstotime a template again. This fixes the behaviour for floats as input.
@ckochendoerfer Can you please approve again? Since Jupp is already on leave but approved before guess he would be alright with templating the function, so we can proceed.

approved this merge request

approved this merge request

added 1 commit

• 962809cb - Make SecondsToTime templated again

Compare with previous version

approved this merge request

approved this merge request

mentioned in commit 840c01ab

merged