Bugfix/v2.0 crash switch #682
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Numerous structs are defined, but none of them have default constructors and there is no guarantee the members will be set when used. With these default constructors, the parameters at least have an initialized value.
Not sure if this was really causing an issue, but proper bound checks are always a good thing.
Something really fishy is going on with the delayBackground function, which will result in crashes when pressing the switch multiple times, with Domoticz MQTT enabled as first controller. Disabled for now and delay(1) added to give background tasks a chance to do their work and make sure the watchdog doesn't perform a reset.
|
Thanks, this is something that I have experienced as well. |
Travis considers them as error and fails the checks.
|
It seems you've made sure all structs are initalized to 0? I looked into this before, and it seemed that structs are always set to 0 by default. (unlike plain old c datatypes). https://stackoverflow.com/questions/706030/are-uninitialized-struct-members-always-set-to-zero Will still merge it offcourse. |
|
Shouldn't those cpp checks be fixed? |
|
I will look into those messages. |
|
https://travis-ci.org/letscontrolit/ESPEasy/jobs/327067956#L541 Its a bit hard to notice since the other warnings are bright and yellow, but are being ignored. :) i already disabled cpp check in the megabranch, because of false positives, but i think this error is actually real. |
TD-er
added a commit
to TD-er/ESPEasy
that referenced
this pull request
Jan 10, 2018
* [crashes] Added constructors to initialize all members in structs Numerous structs are defined, but none of them have default constructors and there is no guarantee the members will be set when used. With these default constructors, the parameters at least have an initialized value. * [PubSubClient] Add bound checks on the internal buffer Not sure if this was really causing an issue, but proper bound checks are always a good thing. * [Crash Switch] Disabled delayBackground and added yield() calls Something really fishy is going on with the delayBackground function, which will result in crashes when pressing the switch multiple times, with Domoticz MQTT enabled as first controller. Disabled for now and delay(1) added to give background tasks a chance to do their work and make sure the watchdog doesn't perform a reset. * [CI build errors] Commented out some unused variables Travis considers them as error and fails the checks. * [CI check] Out-of-bounds check fix
psy0rz
pushed a commit
that referenced
this pull request
Jan 10, 2018
* [switch] Fixed switch behavior and default settings. (#675) As described in #673 . The problem was partly related to the default values stored in flash ("0"), which was not a valid value for the switch type. When upgrading from an older version of ESPeasy, make sure to check the switch type (normal switch or dimmer) and save the settings for the switch device again, even when nothing was changed. Default configuration and new added switches will now work like intended. When a controller is enabled (e.g. Domoticz MQTT or -HTTP) and the button is pressed multiple times, the ESP may reboot. See issue #674. * ABC calibration feature added (#606) * [Flash info] Detailed flash information (#678) Last few days a number of issues and forum topic was about the type of flash used on the ESP boards. This is an extension of the detailed information page. Perhaps also merge with the newer and more clear layout of pull request #624? That pull request was only merged to the mega branch. I kept the changes local, but perhaps they should be placed in the "Storage" section introduced with #624. Maybe also that pull request should get merged into the v2.0 branch. * Bugfix/v2.0 crash switch (#682) * [crashes] Added constructors to initialize all members in structs Numerous structs are defined, but none of them have default constructors and there is no guarantee the members will be set when used. With these default constructors, the parameters at least have an initialized value. * [PubSubClient] Add bound checks on the internal buffer Not sure if this was really causing an issue, but proper bound checks are always a good thing. * [Crash Switch] Disabled delayBackground and added yield() calls Something really fishy is going on with the delayBackground function, which will result in crashes when pressing the switch multiple times, with Domoticz MQTT enabled as first controller. Disabled for now and delay(1) added to give background tasks a chance to do their work and make sure the watchdog doesn't perform a reset. * [CI build errors] Commented out some unused variables Travis considers them as error and fails the checks. * [CI check] Out-of-bounds check fix * actually ignore MQTT messages that are too big. * moved mqtt stuff outside of backgroundtasks(). fixes #683 in my test scenario * [Adafruit MPR121] Change deprecated name setThreshholds to setThresholds (#685) See #684 * fixed plugin id of "Communication - Kamstrup Multical 401". (accidental octal notation) * changed devicecombobox handling to save a lot of memory on device page. fixes #654 #676 and could be triggered by #683 in some cases. * [CPPcheck] v2.0 ControllerSettingsStruct some variables not initialized (#692) Fixing these cppcheck errors: 101.43s$ cppcheck --enable=warning src/*.ino -q --force -I src --include=src/ESPEasy.ino --error-exitcode=1 [src/ESPEasy.ino:500]: (warning) Member variable 'ControllerSettingsStruct::HostName' is not initialized in the constructor. [src/ESPEasy.ino:500]: (warning) Member variable 'ControllerSettingsStruct::Publish' is not initialized in the constructor. [src/ESPEasy.ino:500]: (warning) Member variable 'ControllerSettingsStruct::Subscribe' is not initialized in the constructor.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When pressing the switch very frequently, the ESP device would crash.
Either due to something really fishy in delaying the background tasks (which isn't fixed yet), or the watchdog performing a panic reset of the ESP module.
Is related to #679, #676 and #674 and might be a fix for all of them.