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.

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

Missing stop_reset function #16

Open
riodda opened this issue Dec 1, 2019 · 20 comments
Open

Missing stop_reset function #16

riodda opened this issue Dec 1, 2019 · 20 comments

Comments

@riodda
Copy link

riodda commented Dec 1, 2019

I would suggest to add a function like stop_reset

void Chrono::stop_reset() {
_offset = 0; // reset to zero
_isRunning = false;
}

@sofian
Copy link
Collaborator

sofian commented Dec 1, 2019

It would be useful to have something of the sort. Perhaps the right way to do it would be to have stop() do what you suggest, and replace the current stop() function by pause(). This would break existing code, though. Another option would be to give it as an option to stop() eg. void stop(bool reset=false); so if you want to reset you can call stop(true); What do you think @thomasfredericks ?

@thomasfredericks
Copy link
Collaborator

I suggest to not break the public interface.

Also, I do not like something like stop(true); because it is unclear what we are setting to true.

But since calling start() sets the offset to 0, doesn't it already reset the counter to 0 when started?

For the pause functionality we already have resume().

What is the user case for a Chrono::stop_reset() ?

For reference:

void Chrono::start(Chrono::chrono_t offset) {
  restart(offset);
}

void Chrono::restart(Chrono::chrono_t offset) {
  _startTime = _getTime();
  _offset    = offset;
  _isRunning = true;
}

void Chrono::resume() {
  if (!_isRunning) { // can only resume if was stopped
    _startTime = _getTime();
    _isRunning = true;
  }
}

@sofian
Copy link
Collaborator

sofian commented Dec 2, 2019

I think the problem is that after calling stop() the functions elapsed() and hasPassed() do not return to zero. In some circumstances that can be confusions.

My suggestion: add a function called reset(). It makes sense that a reset() function would also stop the chronometer.

@thomasfredericks
Copy link
Collaborator

There is already a restart(). I believe this might bring feature fatigue.

@sofian
Copy link
Collaborator

sofian commented Dec 2, 2019

@riodda Can you give an example of why you think that feature is important to have?

@sofian
Copy link
Collaborator

sofian commented Dec 2, 2019

@thomasfredericks restart() does not do the same thing, as it starts the timing process. Indeed, there is currently no way to reproduce the initial state of a non-started Chrono object beyond re-calling the constructor.

@sofian
Copy link
Collaborator

sofian commented Dec 4, 2019

Normally real-life chronometers have a reset() function that brings back the counter to 00:00:00. So it would not be too counter-intuitive to have it. I think it makes sense.

@22chrs
Copy link

22chrs commented Mar 8, 2023

This would be really helpful. I have the same problem and I want to reset the Chrono at a given time. My example is, that I want to give the user feedback on a display, that the interval time is starting again. In the first place it was really confusing until I got it, that the restart is not reseting the chrono to 0 again. So having a reset would be really intuitive.
Anyway. Thanks for your great work! :)

@sofian
Copy link
Collaborator

sofian commented Mar 8, 2023

The restart() is resetting the chrono to 0 (basically start() and restart() do the same thing).
We should have had a pause() function from the beginning, and stop() should have reset timer to zero. But at this point I agree we should not break the public interface.

So perhaps a reset() function would be the way to go. I would support adding something like this.

void reset(chrono_t offset=0) {
  _offset = offset; // reset to offset
  _isRunning = false;
}

@thomasfredericks
Copy link
Collaborator

thomasfredericks commented Mar 9, 2023

This would be really helpful. I have the same problem and I want to reset the Chrono at a given time. My example is, that I want to give the user feedback on a display, that the interval time is starting again. In the first place it was really confusing until I got it, that the restart is not reseting the chrono to 0 again. So having a reset would be really intuitive. Anyway. Thanks for your great work! :)

I do not understand. How is restart not setting the Chrono to 0? It should! What do you need?

@sofian
Copy link
Collaborator

sofian commented Mar 9, 2023

@thomasfredericks It's true: restarts() does resets the Chrono to zero -- but it also starts the Chrono. So it is not possible to stop the Chrono (_isRunning = false) and set it to zero (or even, set it to some offset value).

Think about an actual chronometer to time your 100m sprint or whatever. You cannot set it back to zero without actually starting it. I think there are applications where this is something you would want to do. In other words: to me, we are missing the possibility to set the chrono to zero (or some other offset) while also stopping it. We do not have that option right now.

@thomasfredericks
Copy link
Collaborator

thomasfredericks commented Mar 10, 2023

Stop, pause, and play/start behave differently depending on the implementation, the type of device and the manufacturer.

Start
In some cases start continues from a pause. Sometimes start restarts the count from 0.
Pause
Sometimes pause acts as a toggle between a run and not run state. Sometimes it pauses but can't un-pause.
Stop
In some cases stop disables the run state and sets the counter to zero. In other cases it acts as another pause.

In most documentation and examples of the Chrono library, restart is used instead of start as it explicitly states that the counter will be reset to 0 AND the run state will be activated .

Based on wanting to keep this, the other methods could be distinguished as so:

  • stop could deactivate the run state and set the counter to 0
  • pause could deactivate the run state but no set the counter to 0
  • start could activate the run but not reset the counter to 0

Since start could be confused with restart it could be deprecated and renamed continue (but we already have resume).

@thomasfredericks
Copy link
Collaborator

See the documentation here that uses restart() : http://sofapirate.github.io/Chrono/

@sofian
Copy link
Collaborator

sofian commented Mar 11, 2023

Based on wanting to keep this, the other methods could be distinguished as so:

  • stop could deactivate the run state and set the counter to 0
  • pause could deactivate the run state but no set the counter to 0
  • start could activate the run but not reset the counter to 0

Since start could be confused with restart it could be deprecated and renamed continue (but we already have resume).

I think this would be (almost) ideal. But it would break both the behavior of stop() and start() -- so it would not be back-compatible. I see two alternatives:

First proposal (not back-compatible because change to stop() function):

  • stop deactivate the run state and set the counter to 0 (changed)
  • pause deactivate the run state but no set the counter to 0 (new)
  • resume reactivate the run state and no set the counter to 0 (unchanged)
  • start activate the run and reset the counter to 0 (unchanged)

Second proposal (back-compatible):

  • reset set the counter to 0 -- possibly also deactivate the run state (new)
  • stop deactivate the run state but not set the counter to 0 (unchanged) (equivalent of pause() from 1st proposal)
  • resume reactivates the run state and no set the counter to 0 (unchanged)
  • start activates the run and resets the counter to 0 (unchanged)

@thomasfredericks
Copy link
Collaborator

The word reset is too ambiguous. Also as the default behaviour of Chrono is to start running on creation, a reset would actually mean to restart running.

@thomasfredericks
Copy link
Collaborator

A better term could be ready as when runners align on the starting line as in the phrase ready, set, go!.

@sofian
Copy link
Collaborator

sofian commented Mar 11, 2023

Interesting but ready is also a bit vague -- it could also give the impression this is a boolean function.
Another option: we could simply add a function set(chronot_t offset) which would not change the running state but just set current time to a certain value. But then to do a "reset" you need to call set(0). We could make the offset to zero by default so by calling set() would set the clock to 0 -- but it's a big vague.

IMHO the "stop, pause, resume, start" is the clearest approach. It would break back-compatibility so we'd have to be ready for it and it could mean a period of adjustment, but I think it would be my preference, better to do it now than later.

We could (and should) add a set(chronot_t offset) function as well cause it can be useful (right now we only have an add(chronot_t offset) option). But that's another discussion.

@sofian sofian changed the title missin stop_reset function Missing stop_reset function Mar 11, 2023
@thomasfredericks
Copy link
Collaborator

thomasfredericks commented Mar 12, 2023

I would just modifiy the behavoir of stop() (so it resets to 0) and add a new pause() (and probably have to add a togglePause() because someone will expect it to toggle) but then things begin to get complicated so set() is a good compromise. As long as we don't add confusion to or change the behavior of the basic functions elapsed(), restart() and hasPassed().

For @22chrs , the simplest solution would be to just check if the Chrono is running:

if ( myChrono.isRunning() ) Serial.println(0);
else Serial.println(myChrono.elapsed());

@sofian
Copy link
Collaborator

sofian commented Mar 13, 2023

@thomasfredericks Ok, so if I understand right, this would correspond to my first proposal, so it seems we agree:

  • change behavior of stop() so it resets counter to zero
  • add a pause() function

I don't think we need a tooglePause() function because resume() is perfectly consistent IMHO.

But we should definitely add a set() function which seems to be the missing part in our update.

If we do these updates I would suggest we move to version 2.0 because we are breaking compatibility with current version.

@thomasfredericks
Copy link
Collaborator

@sofian, yes I agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants