add different virbrations tunes/patterns support as bzzzt for 50 or 35ms is boring #605

akito45 · 2021-08-20T10:16:40Z

Things attempted

refactor, so that the different vibration types can be declared in one place
support different tunes/patterns, not only durations
try to conserve memory
preserve motorController.RunForDuration(duration) functionality for manual access if needed

src/components/motor/MotorController.cpp

src/components/motor/MotorController.h

Riksu9000 · 2021-08-20T11:12:09Z

src/components/motor/MotorController.cpp

+constexpr MotorController::Tune MotorController::tunes[];
+
+
 MotorController::MotorController(Controllers::Settings& settingsController) : settingsController {settingsController} {
 }

+
+
+uint8_t MotorController::step = 0;
+MotorController::TuneType MotorController::runningTune = MotorController::TuneType::STOP; 
+
+
+


These variables only need to be declared in the header file.

I get undefined reference to Pinetime::Controllers::MotorController::step, etc If I do not initialize it.

Ah that's because these functions and variables are static. Usually in this project something like this is used to get around cases where a callback requires a static function.

void MotorController::Ring(void* p_context) { auto* motorController = static_cast<MotorController*>(p_context); motorController->RunForDuration(50); }

moved step and runningingTune from static scope.
Would like to leave tunes[] as static. The probability is miniscule, that motorController will have multiple instances, but still.

Riksu9000 · 2021-08-20T11:15:51Z

src/components/motor/MotorController.cpp

+  if (step >= tunes[runningTune].length || step >= 8) { //end of tune turn off vibration
+    nrf_gpio_pin_set(pinMotor);
+    return; 
+  }  
+
+  if (((1 << step) & tunes[runningTune].tune) > 0) {
+    nrf_gpio_pin_clear(pinMotor);
+  } else {
+    nrf_gpio_pin_set(pinMotor);
+  }


Any reason not to use RunForDuration() for the patterns as well? It would provide better abstraction.

RunForDuration() is public facing API call, that is currently implemented as "just another tune to play". Would like to not add any more logic to the public facing API. Adding enableMotor() and disableMotor() calls would maybe make it more readable.

RunForDuration() is public facing API call, that is currently implemented as "just another tune to play".

That's because you changed it that way. That function didn't necessarily have to be changed at all.

Refactored the approach to use a unified ScheduleVibrationTimer call.

src/components/motor/MotorController.h

…e, moved some variables from static scope to instance scope

Riksu9000 · 2021-08-20T16:41:01Z

Would it be better to encode patterns as a simple list of delays between motor toggles?

A pattern could be pattern[] = {50, 30, 50, 0}. It would first enable the motor. After 50ms it would toggle the pin and wait for 30ms, effectively adding a silent period. Then after the 30ms the pin would again be toggled, this time enabling the motor, and the next delay would be set to 50ms.

A zero could be used to detect the end of the pattern and stop the motor.

I think doing it this way could be more flexible and simple.

hubmartin · 2021-08-21T07:22:27Z

I find original pattern format (bit pattern, length & tempo) flexible and memory efficient enough. I can imagine that composing will be easier to change bit patterns than copypaste different numbers in an array. I would just increase the uint8_t tune; to uint32_t to have more bits for longer variantions if someone will use 16 or 32 bit pattern instead of just 8.

In case of longer tune the array representation will become worse to read I guess. Also it would need to have every "note" on a different line to see what is length and what is a pause.

pattern[] = {
50, 50, 
150, 50,
50, 50,
150, 50,
50, 50,
150, 50,
}

Now changing the speed will need to change all the values, or at lease 6 of them.

The original approach might look like this and feels cleaner to me

[TuneType::NOTIFICATION] = {.tune = 0b11101011101011101, .length = 17, .tempo = 50},

@akito45 Maybe I would suggest to read the bit pattern from the left (MSB) first. We read text from the left, cursor moves to the right when I press 0 or 1, notes are read from the left to the right, so for composing it will be much better.
Also, you can count the length of the pattern at runtime, no need for length value.
Also you don't have to align patterns exactly to 8 bits, you can write 0b and the exact number of bits you need. Compiler is fine with 0b100 3 bits.

UPDATE:

In hidsight after I tested both options. I changed my mind little and I'm not sure which approach for "composers" will be more friendly :) In case we stick with Riksu suggestion, I would just point out that we might not need millisecond precision and arrays could be uin8_t type if we set let's say 5ms precition (longest vibration will also increase from 255ms to 2.5 sconds), then macro could help us humans, to divide every number by 5.

#define VIBRATE(length, pause) (length / 5), (pause / 5)

pattern[] = {
VIBRATE(50, 50),
VIBRATE(150, 50),
VIBRATE(50, 50),
VIBRATE(150, 50),
VIBRATE(50, 50),
VIBRATE(150, 50)
}

akito45 · 2021-08-21T08:04:57Z

The bit pattern is less wasteful on memory. If made MSB first, the .length could be eliminated, as the length can be calculated on runtime, by just ignoring the preceding 0's.

I would side with the VIBRATE(X, Y) format, if we would have access to some kind of frequency component. The format would make sense for VIBRATE(freq, duration), but not so much for on/off.

Not that familiar with the NRF platform, but can that pin output PWM? In that case VIBRATE(intensity, duration) would make some sense. Pauses could be encoded as VIBRATE(0, duration).

If the majority decides to go with the VIBRATE(X, Y) format, I would advocate for VIBRATE(intencity, duration) for future proofing, not for VIBRATE (on_duration, off_duration) as it seems needlessly custom, for no seeable benefit.

Riksu9000 · 2021-08-21T08:23:33Z

I don't think PWM is necessary because the length of the pulse is practically the same as intensity, and more often than not we would like the vibration to be more noticable than less.

I'm also not sure if we should use a macro to combine the on and off period. We could just decide that one unit is 10ms for example and 10 is easy to work with.

hubmartin · 2021-08-21T08:32:28Z

Right, PWM is possible on any gpio pin but it really does not make any effect on the motor that you would fell. Specially in these short vibrating pulses.

10ms resolution and no macro coulde be fine. However for code readability the macro would be better and self explanatory (with division by 10ms), rather than array of deci-millisecond values.

akito45 · 2021-08-23T17:12:22Z

Either
uint32_t MSB first binary format
[TuneType::NOTIFICATION] = {.tune = 0b11101011101011101, .tempo = 50},
or alternating on / off timing pattern of 10ms
uint8_t pattern[] = {5, 5, 15, 5, 5, 5, 15, 5, 5, 5, 15, 5}

I vote for the first, for memory conservation. It also is more intuitively readable for me. Altho I have 0 musical talent so it is not saying much. Maybe the stakeholders could cast their votes and I could implement the more popular version.

Riksu9000 · 2021-08-23T18:03:54Z

Benefits of using an array of delays are that the code that plays the pattern would be simpler, more efficient and more flexible.

Having a fixed tempo is more limiting, because each vibration has to land on a beat. This could be compensated by increasing the speed, but then the 32 bit limit forces the pattern to be short. Maybe it could be increased to 64 bits, but now it's no longer very memory efficient. The array of delays doesn't have this issue. Patterns can be any length and any precision, making it much more flexible.

The array of delays is harder to read for sure, but splitting the pattern on multiple lines makes it much more readable. Now I can easily tell what this pattern is going to be like.

uint8_t pattern[] = {
	5, 5,
	15, 5,
	5, 5,
	15, 5,
	5, 5,
	15, 0
}

hubmartin · 2021-08-23T18:06:47Z

I'm with riksu with 10ms granuality, so 5 = 50 ms if I got it right.
Every single vibration on a separate line.

uint8_t pattern[] = {
	5, 5,
	15, 5,
	5, 5,
	15, 5,
	5, 5,
	15, 0
}

kieranc · 2022-10-17T08:53:51Z

Any chance of bringing this PR up to date and implementing the requested change to the pattern format @akito45 ?

akito45 · 2022-10-17T12:21:27Z

@kieranc Will blow the dust off the build env in the weekend.

CCranney · 2022-10-26T22:20:11Z

I'd also be interested in this PR being updated - I've subscribed to changes, but thought I'd throw out that there's another interested party

Edit: I'm still very new to PineTime dev, but I read through this pull request and made an updated (but very simplified) version of it for a personal project of mine. I'm not sure if it'll help, but here's a link to the commit in my fork.

agittins · 2022-10-27T09:34:14Z

Two thoughts...

Another option for specifying the pattern could be to approach it like a step-sequencer (old school drum machines etc), and have an int to define step-interval (tempo, if you like) and then 16 or 32bits to define on or off for that step. Some experimentation would be required to see if that many bits is sufficient to give enough pattern length for a given number of intensity durations.

Has anyone actually experimented with pwm on the pinetime motor pin? Varying the pwm frequency (esp at lower freqs) might give usable options. Also, I haven't done the math on it but the pwm controller onboard looks like it might do all the pattern stuff we might want, as it can read step patterns via DMA without loading the cpu - https://infocenter.nordicsemi.com/index.jsp?topic=%2Fcom.nordic.infocenter.nrf52832.ps.v1.1%2Fpwm.html
To be honest I haven't grokked how it all works so I don't know if it can run patterns slowly enough for this purpose, but it does mention LED display patterns in a special mode - the description of which I could not grok - is it perhaps further sub-dividing the clock using the other three patterns?

Edit: This blog post has a little more info including code snippets (between lots of copy/paste from the spec) https://jimmywongiot.com/2021/06/01/advanced-pulse-width-modulation-pwm-on-nordic-nrf52-series/

Also some info on examples (included in the sdk?) at https://infocenter.nordicsemi.com/index.jsp?topic=%2Fcom.nordic.infocenter.sdk5.v11.0.0%2Fpwm_hw_example.html&cp=6_0_0_4_5_15

akito45 added 2 commits August 20, 2021 12:38

add different virbrations support

56f2abd

using new vibrationtunes types

d7a4007

Riksu9000 suggested changes Aug 20, 2021

View reviewed changes

Riksu9000 reviewed Aug 20, 2021

View reviewed changes

src/components/motor/MotorController.h Outdated Show resolved Hide resolved

akito45 added 3 commits August 20, 2021 14:48

cleanups and suggestions

2cd3922

remove STOP state, Schedule next vibration timer tick is more readabl…

fcb57fe

…e, moved some variables from static scope to instance scope

vibrate silence bug

3247001

mruss77 mentioned this pull request Sep 12, 2021

Basic alarm app #662

Merged

Uh oh!

add different virbrations tunes/patterns support as bzzzt for 50 or 35ms is boring #605

Are you sure you want to change the base?

add different virbrations tunes/patterns support as bzzzt for 50 or 35ms is boring #605

Uh oh!

Conversation

akito45 commented Aug 20, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Riksu9000 commented Aug 20, 2021

Uh oh!

hubmartin commented Aug 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

UPDATE:

Uh oh!

akito45 commented Aug 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Riksu9000 commented Aug 21, 2021

Uh oh!

hubmartin commented Aug 21, 2021

Uh oh!

akito45 commented Aug 23, 2021

Uh oh!

Riksu9000 commented Aug 23, 2021

Uh oh!

hubmartin commented Aug 23, 2021

Uh oh!

kieranc commented Oct 17, 2022

Uh oh!

akito45 commented Oct 17, 2022

Uh oh!

CCranney commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agittins commented Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

hubmartin commented Aug 21, 2021 •

edited

Loading

akito45 commented Aug 21, 2021 •

edited

Loading

CCranney commented Oct 26, 2022 •

edited

Loading

agittins commented Oct 27, 2022 •

edited

Loading