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.

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

delayBackground(time) is not working properly and might cause crashes #683

Closed
TD-er opened this issue Jan 7, 2018 · 4 comments
Closed
Labels
Category: Plugin Related to supported sensors Type: Bug Considered a bug
Milestone

Comments

@TD-er
Copy link
Member

TD-er commented Jan 7, 2018

See pull request #682 and issues #679, #676 and #674 and this topic on the forum.

Call to delayBackground() is now removed from the Controller::sendData(), which was probably the most frequently used call to this function.

This function is a wrapper to run backgroundtasks() often while waiting.

It is also called from:

  • deepSleep (in Misc.ino)
  • Command "Delay" for rules (in Command.ino)
  • Controller "Generic HTTP" when sending data from a sensor with multiple values.
  • Controller "Generic UDP" when sending data from a sensor with multiple values.
  • Plugin 015 TSL2561 I2C Lux Sensor when reading data (looks like "delay(...)" could work also here)
  • Plugin 047 Moisture & Temperature & Light I2C Soil moisture sensor when reading data or changing settings (looks like "delay(...)" could work also here)

backgroundtasts() is called from:

  • ExecuteCommand() for the debugging command "background"
  • At the end of the main loop in ESPEasy.ino
  • During scroll animation in Plugin 036: OLED SSD1306 display (see issue Framed Display crashes ESP  #676 )

It looks like calling the backgroundtasks() too often may trigger something strange.

Also strange is the special usecase for Controller[0], which is sometimes hard-coded in the code.
Then calls like this are being made:
getProtocolIndex(Settings.Protocol[0]);
While something like this is to be expected:
getProtocolIndex(Settings.Protocol[event->ControllerIndex]);

@Grovkillen Grovkillen added Type: Bug Considered a bug Category: Plugin Related to supported sensors labels Jan 8, 2018
@psy0rz psy0rz added this to the 2.0.0 milestone Jan 9, 2018
@psy0rz
Copy link
Member

psy0rz commented Jan 10, 2018

Ok i moved the mqtt stuff out of backgroundtasks. This fixes it in my scenario. (domotics MQTT with a message delay of 10000ms)

@TD-er
Copy link
Member Author

TD-er commented Jan 10, 2018

I've looked at the code-change.
Please also test with Domoticz MQTT as first (only?) controller and the switch and press it as fast as you can.
The yield is not always called, since sometimes the time between processing of messages is just about 1 sec (the default delay) and then it will not always call the yield.

@psy0rz
Copy link
Member

psy0rz commented Jan 10, 2018

I did exactly that. Before it crashed immediately, and after the change it seems stable.

If the time is short enough you dont need to call yield. The watchdog will only kick in after 5 or 10 seconds or something.

psy0rz added a commit that referenced this issue Jan 10, 2018
@psy0rz
Copy link
Member

psy0rz commented Jan 10, 2018

together with the memory issue fixes i consider this fixed for now. otherwise please comment/reopen. :)

@psy0rz psy0rz closed this as completed Jan 10, 2018
TD-er pushed a commit to TD-er/ESPEasy that referenced this issue Jan 10, 2018
TD-er pushed a commit to TD-er/ESPEasy that referenced this issue Jan 10, 2018
psy0rz pushed a commit that referenced this issue 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.
TD-er added a commit to TD-er/ESPEasy that referenced this issue Feb 16, 2018
… CPU load

See letscontrolit#869 for discussion on Last Will Topic.
Also changed the way it tried to reconnect to make it return a lot faster when connection is not (yet) possible and call the PubSubClient::loop() at a much slower pace to reduce CPU usage. (See letscontrolit#847)
This higher CPU load was probably introduced when fixing letscontrolit#683.
psy0rz pushed a commit that referenced this issue Feb 16, 2018
* [issue #869] Added 'LWT' to last will topic and improved CPU load

See #869 for discussion on Last Will Topic.
Also changed the way it tried to reconnect to make it return a lot faster when connection is not (yet) possible and call the PubSubClient::loop() at a much slower pace to reduce CPU usage. (See #847)
This higher CPU load was probably introduced when fixing #683.

* [MQTT] Fix error reporting success status with longer payloads

Applied PR https://github.com/knolleary/pubsubclient/pull/360/files

* made MQTT_CALLBACK_SIGNATURE for esp32 functional

Applied PR knolleary/pubsubclient#336
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Plugin Related to supported sensors Type: Bug Considered a bug
Projects
None yet
Development

No branches or pull requests

3 participants