call setTimeout in Countdown ctor #577

muellerr · 2022-03-08T11:55:53+01:00

muellerr commented

2022-03-08 11:55:53 +01:00

I think this is a bit more intuitive. No need to call setTimeout after initializing the countdown with an initial value

muellerr added 1 commit 2022-03-08 11:55:58 +01:00

fsfw/fsfw/pipeline/pr-development This commit looks good Details

238baa8597 call setTimeout

muellerr requested review from gaisser 2022-03-08 11:56:03 +01:00

muellerr added this to the v5.0.0 milestone 2022-03-08 11:56:18 +01:00

gaisser commented

2022-03-09 12:43:02 +01:00

We can do that but it is still necessary to call setTimeout or resetTimer for most apllications because Countdown is typically a member of an object which is created at boot. The time of construction will not be of any use. Even the onboard clock might not be valid at that time.

mohr commented

2022-03-09 18:00:12 +01:00

I agree with Steffen that this is probably not used much, as in a periodic FSW you need to periodically reset your timers anyway.
Do you have a specific use in mind, Robin?

I'm more worried with having unitialized Countdown having an undefined state (ie what is returnd by hasTimedOut()). To have it in a defined state, I would propose to instead call timeOut() in the ctor, to have it be timed out after ctor in most cases (Except for the ones where the time is not initialized yet. But as we get the time from the OS in all current OSALs, I suppose it should be valid at ctor time).

Then we could also remove the warning Otherwise a call to hasTimedOut might return True. from the documentation which quite clearly shows that someone was already thinking about this undefinedness but did not think it trough (assuming that was me, in any other case, please excuse my insinuation).

I agree with Steffen that this is probably not used much, as in a periodic FSW you need to periodically reset your timers anyway. Do you have a specific use in mind, Robin? I'm more worried with having unitialized Countdown having an undefined state (ie what is returnd by `hasTimedOut()`). To have it in a defined state, I would propose to instead call `timeOut()` in the ctor, to have it be timed out after ctor in most cases (Except for the ones where the time is not initialized yet. But as we get the time from the OS in all current OSALs, I suppose it should be valid at ctor time). Then we could also remove the warning `Otherwise a call to hasTimedOut might return True.` from the documentation which quite clearly shows that someone was already thinking about this undefinedness but did not think it trough (assuming that was me, in any other case, please excuse my insinuation).

👍 1

muellerr commented

2022-03-10 10:12:00 +01:00

Yes, I have a specific use-case and I use this in the EIVE OBSW.

#if OBSW_ENABLE_TIMERS == 1
  Countdown timer(1000);
#endif
  while (true) {
    ReturnValue_t result = cmdExecutor.check(bytesRead);
    // This timer can prevent deadlocks due to missconfigurations
#if OBSW_ENABLE_TIMERS == 1
    if (timer.hasTimedOut()) {
      sif::error << "SdCardManager::checkCurrentOp: Timeout!" << std::endl;
      return OpStatus::FAIL;
    }
#endif

if a variant which does not set up the start time is required, it might also make more sense to define a constructor which does not take an initial timeout. From an C++ API perpective, the code above is the most inutitive for me, no need to call setTimeout again, simply create the countdown on stack and it's ready to use.

Yes, I have a specific use-case and I use this in the EIVE OBSW. ```cpp #if OBSW_ENABLE_TIMERS == 1 Countdown timer(1000); #endif while (true) { ReturnValue_t result = cmdExecutor.check(bytesRead); // This timer can prevent deadlocks due to missconfigurations #if OBSW_ENABLE_TIMERS == 1 if (timer.hasTimedOut()) { sif::error << "SdCardManager::checkCurrentOp: Timeout!" << std::endl; return OpStatus::FAIL; } #endif ``` if a variant which does not set up the start time is required, it might also make more sense to define a constructor which does not take an initial timeout. From an C++ API perpective, the code above is the most inutitive for me, no need to call setTimeout again, simply create the countdown on stack and it's ready to use.

mohr commented

2022-03-10 13:26:55 +01:00

Ok in summary:

Proposed change is useful when Countdown is used on stack (which at least one user does, EIVE)
Proposed change does not break anything for current use
I do agree that espiecially if used on the stack, the proposed change feels quit C++y
Having defined state is nice in theory, but not in current implementation anyway and conflicts with actual use case for proposed change.

As such I am in favour of merging this PR.

Ok in summary: * Proposed change is useful when Countdown is used on stack (which at least one user does, EIVE) * Proposed change does not break anything for current use * I do agree that espiecially if used on the stack, the proposed change feels quit C++y * Having defined state is nice in theory, but not in current implementation anyway and conflicts with actual use case for proposed change. As such I am in favour of merging this PR.

👍 2

gaisser approved these changes 2022-03-14 14:31:11 +01:00

gaisser added 1 commit 2022-03-14 14:47:01 +01:00