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

Config does not contain a settings object #323

Closed
Gulaschcowboy opened this issue Mar 27, 2017 · 17 comments
Closed

Config does not contain a settings object #323

Gulaschcowboy opened this issue Mar 27, 2017 · 17 comments

Comments

@Gulaschcowboy
Copy link

Gulaschcowboy commented Mar 27, 2017

Hi @marvinroger et al.

I have defined 4 custom settings and I can't update them via MQTT nor via App/Web. To narrow down the issue I tried same with your CustomSettings example. If I don't change it and keep just 1 custom setting I can update the settings with :
mosquitto_pub -h broker -t "homie/test-3/$implementation/config/set" -u user -P pass -m '{"settings":{"temperatureInterval":30}}' -r

As soon as I add my settings it fails. After publishing with the above command I get
✖ Config does not contain a settings object
✖ Configuration not updated
in serial monitor.

See my modified CustomSettings example attached

test-settings.ino.txt

Update:
I could narrow it down further. If I don't set more than 1 value as default, all values become settable.

Thank you
Alex

@exxamalte
Copy link

Just looking at the code (Config.cpp, lines 246-263), it appears that the error message "Config does not contain a settings object" is thrown when trying to set config values but settings is not defined yet, i.e. had not been defined before.
Does the config file on your device contain a settings key initially?

@Gulaschcowboy
Copy link
Author

@exxamalte "Does the config file on your device contain a settings key initially?" Yes, this will be the case, but it is just the resulting error.
As I stated in my Update: If you define more than one HomieSetting with a default value via setDefaultValue the error occurs. To reproduce: Define more than one HomieSetting, make more than one defined with a default value using setDefaultValue. Set values in the App/Web different from the defaults and those manual, non-default values are not used during runtime and are not settable via MQTT $configuration/set.

Don't use setDefaultValue or only once and they become settable via APP/Web and MQTT. So my uneducated guess is, that there is a parsing error in /Utils/Validation.cpp

@kartom
Copy link
Contributor

kartom commented Apr 9, 2017

I can confirm that custom settings does not work when the default values are set. It works fine when not using the default values.
It would also be good if the custom settings of type double would accept integer values, like 12 instead of 12.0. The app does not support writing 12.0, it will automatically be converted to 12, so you have to enter 12.01 or similar to pass the validation.

@marvinroger marvinroger added this to the v2.0.0 milestone Apr 19, 2017
@marvinroger
Copy link
Member

I can't reproduce. Sample sketch:

#include <Homie.h>

HomieNode temperatureNode("temperature", "temperature");

HomieSetting<long> aSetting("a", "A");
HomieSetting<long> bSetting("b", "B");

void setup() {
  Serial.begin(115200);
  Serial << endl << endl;
  Homie_setFirmware("temperature-setting", "1.0.0");

  temperatureNode.advertise("unit");
  temperatureNode.advertise("degrees");

  aSetting.setDefaultValue(1);
  bSetting.setDefaultValue(2);

  Homie.setup();
}

void loop() {
  Homie.loop();
}

I initially set a to 2 and b to 3 in the app, and these values are loaded properly (update to latest git to see the value of custom settings in the log). And updating these settings through MQTT is working fine.

The Config does not contain a settings object you're having makes me think that your ArduinoJson is broken. Can you update to ArduinoJson v2.8.4?

@kartom
Copy link
Contributor

kartom commented Apr 19, 2017

@marvinroger: Thanks a lot for spending time testing this, and for the nice work with the whole framework!

However I can reproduce the problem in two different ways (using ArduinoJson v2.8.4):

Way 1:

  1. Leave the default values in the app.
  2. Try to change the settings via MQTT.

Way 2:

  1. Created a sketch without the HomieSettings.
  2. Configure the device
  3. Add the HomieSettings to the sketch and update the ISP
  4. Try to change the settings via MQTT

I think both these two problems can be solved by adding an empty "settings" in the config.json if there are no settings or just default values. I believe this because as soon as you have one setting in the "settings" section of the config.json then all others settings can be set from MQTT, regardless if the setting exists or not.

One solution would be to add something similar to this before line 369 in BootConfig.cpp:
if(!parsedJson.containsKey("settings")) { parsedJson.createNestedObject("settings"); }

@kartom
Copy link
Contributor

kartom commented Apr 19, 2017

Suggestion:

Change line 316 in Validation.cpp from:
if (!(*settingsObject)[setting->getName()].is<double>()) {
to:
if ((!(*settingsObject)[setting->getName()].is<double>()) && (!(*settingsObject)[setting->getName()].is<long>())) {

This might solve the problem that you cant enter a value without decimals as a double in the config app. (The value 1.0 will not work either...)

@marvinroger
Copy link
Member

Oh, okay.

Your suggestion would work fine if everyone would load the configuration boot, but you can also manually flash the configuration file to the SPIFFS, so it won't work.

This bug was introduced when making the settings key optional. I'm all for restoring the initial behavior, it avoids adding some boilerplate code, and the end dev only has to add a settings key to its config file, otherwise it would fail at validation. Would it work for you?

@kartom
Copy link
Contributor

kartom commented Apr 20, 2017

I am not sure that I fully understand your question.
If you mean that the settings section always will be present, even tough it' empty, and the configuration boot will handle this. The only difference from "user" perspective is that if you would like to flash a configuration file, it will be mandatory to include the settings section, otherwise the configuration file won't work?
If that is the case, and it also saves some code, I'm definitely positive to that.

@marvinroger
Copy link
Member

Yes, this is what I meant! 😉 Great.

@kartom
Copy link
Contributor

kartom commented Apr 20, 2017

You might get the possibility to flash configuration files without the settings section if you add something similar to what I suggested just before the validation. In this way you can skip the settings section in a manually flashed file if you are not using settings at all. But if you do that, then there is of course impossible to change the settings from MQTT, which in my meaning makes perfectly sense, especially in combination with the error message: The settings section is missing...

But as I said above, just remove the opportunity to have a configuration file without settings section is perfectly fine for me...

Have you got the time to look into the suggestion about the double validation problem?

@luebbe
Copy link
Collaborator

luebbe commented May 8, 2017

I think that @kartom s suggestion for the double validation problem will also fix #332

@marvinroger marvinroger modified the milestones: v2.0.0-beta.2, v2.0.0 Jun 1, 2017
@CapnBry
Copy link

CapnBry commented Jul 7, 2017

I was coming here to report this as well! I deployed my Homie 2.0 nodes with default HomieSetting values with the notion that I could tweak them as needed, however now I can not because none of them have a "settings" key in config.json.

I feel like Homie should not require the configuration file to have this key before it will allow writing to it. If you leave the settings the default during provisioning, you can't change them without editing the settings file somehow? Either BootNormal or Config::patch should allow the special case of patching in a NEW settings top-level key. This of course also begs the question how to get back the default values since you can't patch out any values, only change them.

@fabiosoft
Copy link

Note that in initial config (configuration mode/fresh reset) there must be a
settings: {} object in config.json or it will rise an error.

@timpur
Copy link
Contributor

timpur commented Jan 2, 2018

I ran into this problem and should be fixed in #458

@stritti
Copy link
Collaborator

stritti commented Apr 4, 2019

@timpur and @Gulaschcowboy could we close this issue? Is it solved?

@kleini
Copy link
Collaborator

kleini commented Apr 4, 2019

{"name":"Temperature 002","device_id":"temp-002","wifi":{"ssid":"Klein2","ip":"192.168.168.195","mask":"255.255.255.0","gw":"192.168.168.1","dns1":"192.168.168.1"},"mqtt":{"host":"192.168.168.6","port":1883,"auth":true,"ssl":true},"ota":{"enabled":true},"settings":{"temperatureOffset":-0.1}}

I can confirm, that the config contains the settings object with current develop branch.

@kleini kleini closed this as completed Apr 4, 2019
@kartom
Copy link
Contributor

kartom commented Jul 8, 2019

The error still occurs if you add settings after initial configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants