crownstone / bluenet

Bluenet is the in-house firmware on Crownstone hardware. Functions: switching, dimming, energy monitoring, presence detection, indoor localization, switchcraft.
https://crownstone.rocks
91 stars 62 forks source link

Bugfix. Firmware crash on board.pinDimmer = PIN_NONE #166

Closed mrquincle closed 2 years ago

mrquincle commented 2 years ago

Initialized is set to true early in init() function. Only after the dimmer has been initialized should this be set to true.

Within init() when !_hasDimmer the function returns. It is not known to the rest of the firmware if initialization has been successful.

The current code calls Dimmer::start() even if there's no dimmer. This crashes the firmware.

The current patch is by adding early returns when !_hasDimmer for the other dimmer functions. This won't have a negative effect on the current code and leads to minimum code changes for the code that uses this class.

The patch is motivated by being able to have firmware in the field without IGBTs (e.g. smart outlets) and introduces minimal code changes.

Alternatively, we can:

  1. Dynamically create a dimmer instance depending on its presence.
  2. Remove it as a singleton.
  3. Guard all calls by functionality that checks if a dimmer is present. With init(), with start(), with set(), with setSoftOnSpeed(), and with enable().

Until then I'd recommend this small patch.

ArrowAcrobatics commented 2 years ago

Made a small commit to update code style and fix up _started and _initialized check in start() and init().

The asserts look a little random indeed -- considering that _started and _enabled don't have similar asserts. The assert did it's job though, as it pointed a developer to a problem rather than swallowing the exception and 'just doing nothing'.

Underlying issue: init flow of the switch could be cleaned up to be more defensive against faulty configs. Priority low I'd say.

Can be merged and closed after tests.

vliedel commented 2 years ago

This bug probably only occurred for dev boards, as they start the dimmer without waiting for a zero crossing.

mrquincle commented 2 years ago

Happened to the prototype by @iesverhage.