Fix unsigned/signed comparison warning in Metronome.cpp

[NeroBurner]

xTaskGetTickCount() returns a TickType_t, which is defined as an
uint32_t. This is compared to the bpm variable, which is a int16_t
in the range of 40 to 220 as defined in the constructor.

  lv_arc_set_range(bpmArc, 40, 220);

Just to be sure also add an assert that bpm is greater than 0, as this
would result in a division by zero or negative values, which would
unintentionally underflow to a very large number.

---

Not flashed, I have only a sealed watch and I'm afraid of bricking it

[Avamander]

The assert is not ideal in this scenario. It would be better to turn BPM into an uint16_t.

[evergreen22]

The best solution IMO would be to promote bpm to a uint32_t before the comparision. There would be no loss of information this way and it would eliminate the warning.

[Avamander]

@evergreen22 why the unnecessary precision?

[evergreen22]

The problem as I understand it as a uint32_t is being compared to a uint16_t. This causes a compiler warning. Possible solutions are: * convert the 32 bit value to 16 bit * convert the 16 bit value to 32 bit * ignore the warning * change the declared types to match The best choice is the last one. However, if we don't want to do that I think the next safest choice is the second one. Internally all comparisions are done with ints so that is why I suggested promotion. I would note that there is a risk of hitting undefined behavior with unsigned variables and promotion that may result in undefined behavior depending on how the compiler handles promotion of unsigned ints. IDK if that is a problem with our compiler. “There are far too many integer types, there are far too lenient rules for mixing them together, and it’s a major bug source, which is why I’m saying stay as simple as you can, use [signed] integers til you really really need something else.” -Bjarne Stroustrup

[Avamander]

The best choice is the last one. However, if we don't want to do that I think the next safest choice is the second one.

Well it kinda is the only choice if I'm being totally honest. The goal of the warning or removal of it shouldn't be simply getting rid of the warning, but to warn against and fix inconsistencies in integer typing.

[NeroBurner]

as clarification: the warning is about the signed int (bpm as int16_t) and unsigned int (TickType_t aka uint32_t) comparison, not the different bit-sizes

I've change the bpm type to be uin32_t.

When getting the bpm value from the arc convert the returned value from int16_t to unit32_t without a check (should I add a comment, that we assume the return value to be strictly positive?).

int16_t lv_arc_get_value(const lv_obj_t * arc)

When setting the arc-value convert from uint32_t to the expected int16_t (as we got the value from the arc in the first place I think it ok to assume the bpm value is still in the int16_t range)

void lv_arc_set_value(lv_obj_t * arc, int16_t value)

[Riksu9000]

I don't think bpm type should be changed. LVGL works with 16 bit ints and TickType_t is 32 bit uint, so they have to mix at some point. If we change bpm type, we have to cast it everywhere instead of just at the one point. The first commit was fine, just without the assert.

[NeroBurner]

@Riksu9000 updated

[JF002]

Thanks!