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

timer: Add ringing and counter #1971

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

timer: Add ringing and counter #1971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions src/components/motor/MotorController.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ void MotorController::StopRinging() {
nrf_gpio_pin_set(PinMap::Motor);
}

bool MotorController::IsRinging() {
return (xTimerIsTimerActive(longVib) == pdTRUE);
}

void MotorController::StopMotor(TimerHandle_t /*xTimer*/) {
nrf_gpio_pin_set(PinMap::Motor);
}
1 change: 1 addition & 0 deletions src/components/motor/MotorController.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ namespace Pinetime {
void RunForDuration(uint8_t motorDuration);
void StartRinging();
void StopRinging();
bool IsRinging();

private:
static void Ring(TimerHandle_t xTimer);
Expand Down
16 changes: 13 additions & 3 deletions src/components/timer/Timer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,13 @@ void Timer::StartTimer(std::chrono::milliseconds duration) {
}

std::chrono::milliseconds Timer::GetTimeRemaining() {
TickType_t remainingTime = 0;
if (IsRunning()) {
TickType_t remainingTime = xTimerGetExpiryTime(timer) - xTaskGetTickCount();
return std::chrono::milliseconds(remainingTime * 1000 / configTICK_RATE_HZ);
remainingTime = xTimerGetExpiryTime(timer) - xTaskGetTickCount();
Copy link
Member

Choose a reason for hiding this comment

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

On further reflection, we shouldn't change the semantics of this method

The Timer component is meant to be a generic component that multiple applications can be use (it should be just a thin wrapper around FreeRTOS timers)

One idea:

  • When setting a timer, the expiry tick time is stored in this class
  • New method GetTimeSinceExpired which returns xtaskgettickcount - expiry, or 0 (if running or stopped)
  • This method remains pretty much unchanged
  • Inside the timer screen we calculate the value as GetTimeRemaining if the timer is running, else with GetTimeSinceExpired

The only part of this I don't really like is the semantics of GetTimeSinceExpired when it hasn't expired yet or has been stopped, and also GetTimeRemaining when stopped or expired

Another idea:
Method GetTimerStatus which returns a variant of

  • Stopped
  • Running: time to expiry
  • Expired: time since expiry

This way semantics are always clear

Curious to know what you're thinking, any of these make sense? Not sure I've got any perfect solutions here, though I quite like the second one so far (haven't thought about either for too long)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Timer component is meant to be a generic component that multiple applications can be use (it should be just a thin wrapper around FreeRTOS timers)

Is it, though? That's what the xTimer interface is meant to do. This wrapper into a Timer component is only used for the Timer app. That said, I think this Timer class could still work elsewhere in the code as is, if there was a use case for it. If a new use case shows up, and a change is needed, we can do it then - not sure creating a super generic class is useful without knowing what use cases it's meant to address.

That's just my opinion though, and I may be entirely wrong (and I'm just having a hard time finding time to work on this these days - which is why I'm hesitant to get started on a change I probably won't finish).

} else if (expired > 0) {
remainingTime = xTaskGetTickCount() - expired;
}
return std::chrono::milliseconds(0);
return std::chrono::milliseconds(remainingTime * 1000 / configTICK_RATE_HZ);
}

void Timer::StopTimer() {
Expand All @@ -26,3 +28,11 @@ void Timer::StopTimer() {
bool Timer::IsRunning() {
return (xTimerIsTimerActive(timer) == pdTRUE);
}

void Timer::SetExpiredTime() {
expired = xTimerGetExpiryTime(timer);
}

void Timer::ResetExpiredTime() {
expired = 0;
}
5 changes: 5 additions & 0 deletions src/components/timer/Timer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,13 @@ namespace Pinetime {

bool IsRunning();

void SetExpiredTime();

void ResetExpiredTime();

private:
TimerHandle_t timer;
TickType_t expired = 0;
};
}
}
11 changes: 7 additions & 4 deletions src/displayapp/DisplayApp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,14 +365,17 @@ void DisplayApp::Refresh() {
if (state != States::Running) {
PushMessageToSystemTask(System::Messages::GoToRunning);
}
// Load timer app if not loaded
if (currentApp != Apps::Timer) {
LoadNewScreen(Apps::Timer, DisplayApp::FullRefreshDirections::Up);
}
// Once loaded, set the timer to ringing mode
if (currentApp == Apps::Timer) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this check redundant as timer is now loaded before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is redundant, but the builder fails when initializing the *timer screen as a direct clause of the case. Kind of baffled about this one, but I can only build successfully if I check that the current app is the Timer before I try to initialize the Timer screen. 🤷

lv_disp_trig_activity(nullptr);
auto* timer = static_cast<Screens::Timer*>(currentScreen.get());
timer->Reset();
} else {
LoadNewScreen(Apps::Timer, DisplayApp::FullRefreshDirections::Up);
timer->SetTimerRinging();
}
motorController.RunForDuration(35);
motorController.StartRinging();
break;
case Messages::AlarmTriggered:
if (currentApp == Apps::Alarm) {
Expand Down
37 changes: 33 additions & 4 deletions src/displayapp/screens/Timer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ static void btnEventHandler(lv_obj_t* obj, lv_event_t event) {
}
}

Timer::Timer(Controllers::Timer& timerController) : timer {timerController} {
Timer::Timer(Controllers::Timer& timerController, Controllers::MotorController& motorController)
: timer {timerController}, motorController {motorController} {

lv_obj_t* colonLabel = lv_label_create(lv_scr_act(), nullptr);
lv_obj_set_style_local_text_font(colonLabel, LV_LABEL_PART_MAIN, LV_STATE_DEFAULT, &jetbrains_mono_76);
Expand Down Expand Up @@ -62,7 +63,9 @@ Timer::Timer(Controllers::Timer& timerController) : timer {timerController} {
txtPlayPause = lv_label_create(lv_scr_act(), nullptr);
lv_obj_align(txtPlayPause, btnPlayPause, LV_ALIGN_CENTER, 0, 0);

if (timer.IsRunning()) {
if (motorController.IsRinging()) {
SetTimerRinging();
} else if (timer.IsRunning()) {
SetTimerRunning();
} else {
SetTimerStopped();
Expand Down Expand Up @@ -103,7 +106,17 @@ void Timer::UpdateMask() {
}

void Timer::Refresh() {
if (timer.IsRunning()) {
if (isRinging) {
DisplayTime();
// Stop buzzing after 10 seconds, but continue the counter
if (motorController.IsRinging() && displaySeconds.Get().count() > 10) {
motorController.StopRinging();
}
// Reset timer after 1 minute
if (displaySeconds.Get().count() > 60) {
Reset();
}
} else if (timer.IsRunning()) {
DisplayTime();
} else if (buttonPressing && xTaskGetTickCount() > pressTime + pdMS_TO_TICKS(150)) {
lv_label_set_text_static(txtPlayPause, "Reset");
Expand All @@ -129,16 +142,31 @@ void Timer::SetTimerRunning() {
minuteCounter.HideControls();
secondCounter.HideControls();
lv_label_set_text_static(txtPlayPause, "Pause");
lv_obj_set_style_local_bg_color(btnPlayPause, LV_BTN_PART_MAIN, LV_STATE_DEFAULT, Colors::bgAlt);
}

void Timer::SetTimerStopped() {
isRinging = false;
minuteCounter.ShowControls();
secondCounter.ShowControls();
lv_label_set_text_static(txtPlayPause, "Start");
lv_obj_set_style_local_bg_color(btnPlayPause, LV_BTN_PART_MAIN, LV_STATE_DEFAULT, LV_COLOR_GREEN);
}

void Timer::SetTimerRinging() {
isRinging = true;
minuteCounter.HideControls();
secondCounter.HideControls();
lv_label_set_text_static(txtPlayPause, "Reset");
lv_obj_set_style_local_bg_color(btnPlayPause, LV_BTN_PART_MAIN, LV_STATE_DEFAULT, LV_COLOR_RED);
timer.SetExpiredTime();
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately I think this is still UB as the timer is no longer running at this point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without calling this, there's no count-up in the timer: it just stays at 00:00 while ringing. Not sure I want to skip this feature - I find it way too useful - and so you need to tell the Timer component what the expiry time is so that the GetTimeRemaining call returns the correct value (otherwise expired remains at 0 and nothing happens).

}

void Timer::ToggleRunning() {
if (timer.IsRunning()) {
if (isRinging) {
motorController.StopRinging();
Reset();
} else if (timer.IsRunning()) {
DisplayTime();
timer.StopTimer();
SetTimerStopped();
Expand All @@ -151,6 +179,7 @@ void Timer::ToggleRunning() {
}

void Timer::Reset() {
timer.ResetExpiredTime();
DisplayTime();
SetTimerStopped();
}
8 changes: 6 additions & 2 deletions src/displayapp/screens/Timer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#pragma once

#include "displayapp/screens/Screen.h"
#include "components/motor/MotorController.h"
#include "systemtask/SystemTask.h"
#include "displayapp/LittleVgl.h"
#include "displayapp/widgets/Counter.h"
Expand All @@ -14,20 +15,22 @@ namespace Pinetime::Applications {
namespace Screens {
class Timer : public Screen {
public:
Timer(Controllers::Timer& timerController);
Timer(Controllers::Timer& timerController, Controllers::MotorController& motorController);
~Timer() override;
void Refresh() override;
void Reset();
void ToggleRunning();
void ButtonPressed();
void MaskReset();
void SetTimerRinging();

private:
void SetTimerRunning();
void SetTimerStopped();
void UpdateMask();
void DisplayTime();
Pinetime::Controllers::Timer& timer;
Pinetime::Controllers::MotorController& motorController;

lv_obj_t* btnPlayPause;
lv_obj_t* txtPlayPause;
Expand All @@ -42,6 +45,7 @@ namespace Pinetime::Applications {
Widgets::Counter secondCounter = Widgets::Counter(0, 59, jetbrains_mono_76);

bool buttonPressing = false;
bool isRinging = false;
lv_coord_t maskPosition = 0;
TickType_t pressTime = 0;
Utility::DirtyValue<std::chrono::seconds> displaySeconds;
Expand All @@ -54,7 +58,7 @@ namespace Pinetime::Applications {
static constexpr const char* icon = Screens::Symbols::hourGlass;

static Screens::Screen* Create(AppControllers& controllers) {
return new Screens::Timer(controllers.timer);
return new Screens::Timer(controllers.timer, controllers.motorController);
};
};
}
Loading