Pid branch revived

[marcvs]

Description:

This picks up on the pid_branch of Colinl. That branch didn't compile on tasmota 9. This updated version fixes that.
See also:

• Original bug: Does not compile with tasmota 9.2.0 colinl/Sonoff-Tasmota#15
• PR against colinl/Sonoff-Tasmota pid_branch (this shows the changes I made vs. the existing pid code) Pid branch to address issue #15 colinl/Sonoff-Tasmota#16

Checklist:

• The pull request is done against the latest development branch
• Only relevant files were touched
• Only one feature/fix was added per PR and the code change compiles without warnings
• The code change is tested and works on Tasmota core ESP8266 V.2.7.4.9
  • I didn't find any unit tests to run, but I tested it on my ESP (sonoff-mini), and it yields the same results as the original patch.
• The code change is tested and works on Tasmota core ESP32 V.1.0.5-rc4
  • I don't have an ESP32 available.
• I accept the CLA.

[Jason2866]

@marcvs That was quick :-) Great work. Two little things from my side. My advice to use the lib folder default was to get your starting issues quickly solved. Please move the lib, from folder default to folder lib_div Dont worry, no other changes needed.
Second one, please add the needed #define xxx to activate your driver commented to my_user_config.h
THX!

[marcvs]

Will to.

One Question:
There is a lot of options for setting-up PID variables. The PID section of my config has 111 lines, describing 23 Variables. The additional lines are extensive information, which is also included on top of the two added xdrv_9[12]*.ino files.
Basically the question is, whether it's good practice to leave only USE_TIMEPROP and USE_PIP an in the comment refer to additional parameters and documentation on top of xdrv_91_timeprop.ino and xdr_92_pid.ino.

[marcvs]

Also: The inclrease in code size is measured on what?

• tasmota.bin
• tasmota.bin.gz
• tasmota.elf
  ?

[Jason2866]

Every useful Info in the source code is good. In my_user_config.h only the needed settings and a very short explanation.
Codesize increase is measued with standard tasmota.bin with and without the new driver

[marcvs]

Ok; There we go.

[arendst]

Thx.

I can see this is very "old" code with lots of missing optimizations leading to larger than needed code footprint. Tasmota has gone a long way since this code was conceived. I could merge but than have to optimize a lot so I request you to do it.

I suggest the following changes before I merge:

• Change any boolean to bool.
• All AddLog_P text needs to start with a fixed three letter code and colon to identify it from other logging messages. In case of PID just use PID:.
• Lower the number of logging messages as they take up code space.
• Revisit the command handling and change it to using DecodeCommand by looking at ie xdrv_09-timers.ino.
• Change commands to Tasmota style commands: pid_pv should become PidPv. No underlines. In this case use Pid as prefix. See xdrv_07_domoticz.ino how to use this.
• Provide result of command as MQTT message instead of Logging messages. Change used AddLog_P to Response_P and use as much as possible predefined Response.. functions. (Search for void ResponseCmndNumber to see available functions).
• Remove any access to TasmotaGlobal.mqtt_data as it can be easily changed to use Response_P and it's supporting functions.
• Change driver index number to xdrv_48 and if needed xdrv_49.

I know it's some work to do but it makes it a muche better candidate for merging.

Good luck, Theo.

[marcvs]

Thanks for the pointers; I'll get into this either tomorrow or thursday.

[marcvs]

Question:

Remove any access to TasmotaGlobal.mqtt_data as it can be easily changed to use Response_P and it's supporting functions.

Currently I'm accessing TasmotaGlobl.mqtt_data for two purposes: read sensor data from DS18B20, and for writing output.
I understand your comment for the output part.
What is the suggested way to read the sensor data?

[arendst]

Great.

For the DS18B20 temperature use global (float) variable TasmotaGlobal.temperature_celsius. This is updated by any type of connected temperature sensor so you do not have to worry about sensors connected.

[marcvs]

Nice! That simplifies the code a lot!
How can I add variables to the SENSOR topic? I would like to #define PID_EXTRA_OUTPUT in which case I'll add the relevant variables to the sensor.

[arendst]

Search for call FUNC_JSON_APPEND to append JSON data to sensor output.

[marcvs]

• Change any boolean to bool.
• All AddLog_P text needs to start with a fixed three letter code and colon to identify it from other logging messages. In case of PID just use PID:.
• Lower the number of logging messages as they take up code space.
• Revisit the command handling and change it to using DecodeCommand by
  looking at ie xdrv_09-timers.ino.
• Change commands to Tasmota style commands: pid_pv should become PidPv. No
  underlines. In this case use Pid as prefix. See xdrv_07_domoticz.ino how
  to use this.
• Provide result of command as MQTT message instead of Logging messages.
  Change used AddLog_P to Response_P and use as much as possible predefined
  Response.. functions. (Search for void ResponseCmndNumber to see available
  functions).
• Remove any access to TasmotaGlobal.mqtt_data as it can be easily changed
  to use Response_P and it's supporting functions.

I kept one occurence where the current pid power is published. I've barred it
behind a default-off ifdef, for backward compatibility, though.

• Change driver index number to xdrv_48 and if needed xdrv_49.

I was not sure about some code in xdrv_48_timeprop.ino that @colinl wrote. I've removed it, since my tests showed no changes in function.

[Jason2866]

@marcvs It would be great if you would update the development path of the Tasmota docu.
https://github.com/tasmota/docs/blob/development/docs/PID-Control.md
Clicking the pencil will generate a fork and you can a provide a PR.
The changes made to the docu development path will be "visible" when a new release is published

Thx!

[colinl]

@marcvs you appear to have removed the code for setting the timeprop power via MQTT where it is desired to use the timeprop output without the PID feature, or am I missing something?

[colinl]

@marcvs in xdrv_49_pid.ino the code to publish the power via MQTT has been placed inside a backwards compatibility #ifdef, however access to the power is vital for loop tuning so I suggest that this should not be conditionally compiled.

[marcvs]

Yes, I didn't find that documented, and hence was not sure if this was a stale part or not.

I can move that back in. The problem was that I had to move towards using DecodeCommand which didn't work as expected (and I had to get done).

I will take another session on this soon (towards the weekend)

[colinl]

It is documented in the comments at the start of timeprop.ino.

 * In the case where only one relay is being driven the power value is set by
 * writing the value to the mqtt topic cmnd/timeprop_setpower_0.
...

Did you see my comment above about publishing the power value? Perhaps it is being published somewhere but I can't see it.

[marcvs]

Yes, I found that. I'll fix it.
I also found the annoyance that my code does not compile as tasmota-minimal.
The last things missing are:

• Use DecodeCommand in xdrv_48_timeprop.ino
• Update the documentation.
  Planning on doing this over the weekend.

[marcvs]

Hi There,

I'm a bit stuck on including the code that I had earlier removed. I need some instructions for:

In xdrv_48_timeprop.ino, there is advanced handling of incoming commands (in bool Timeprop_Command), so that the timeprop_setvalue_x can be received to set the timeprop value for relay x. Also it is being used from xdrv_49_pid.ino.
Is it really necessary to move to DecodeCommand? If so: How?

In xdrv_49_pid.ino I did comment-out-by-default the feature that reports the pid power to /PID. Apparently this is being used and required. The frequency is different to the Teleperiod, as this one changes on the update_secs interval.
I did comment it out because it accesses TasmotaGlobal.mqtt_data which is suggested no to be done.
What shall I do here?

[karlkashofer]

Hi Marcvs ! Any news on this ? I would be very interested in a working PID, and be happy to test it on my sonoffs.
Cheers,
KK

[marcvs]

This was merged in 9.3. Please check the configuration in the comments at the start of these files:

• https://github.com/arendst/Tasmota/blob/development/tasmota/xdrv_49_pid.ino
• https://github.com/arendst/Tasmota/blob/development/tasmota/xdrv_48_timeprop.ino

[karlkashofer]

Sorry for being a bit slow here, but that does not seem to work:
git clone https://github.com/arendst/Tasmota.git
cp Tasmota/tasmota/user_config_override_sample.h Tasmota/tasmota/user_config_override.h
vi Tasmota/tasmota/user_config_override.h (add #define USE_TIMEPROP #define USE_PID at the end )
docker run -ti --rm -v $(pwd)/Tasmota:/tasmota -u $UID:$GID docker-tasmota -e tasmota-sensors

results in:

---

Compiling .pio/build/tasmota-sensors/lib512/ESP8266WiFi/ESP8266WiFiAP.cpp.o
Compiling .pio/build/tasmota-sensors/lib512/ESP8266WiFi/ESP8266WiFiGeneric.cpp.o
Compiling .pio/build/tasmota-sensors/lib512/ESP8266WiFi/ESP8266WiFiGratuitous.cpp.o
Compiling .pio/build/tasmota-sensors/lib512/ESP8266WiFi/ESP8266WiFiMulti.cpp.o
Compiling .pio/build/tasmota-sensors/lib512/ESP8266WiFi/ESP8266WiFiSTA-WPS.cpp.o
/tasmota/tasmota/xdrv_48_timeprop.ino:96:27: error: 'TIMEPROP_NUM_OUTPUTS' was not declared in this scope
static Timeprop timeprops[TIMEPROP_NUM_OUTPUTS];
^
/tasmota/tasmota/xdrv_48_timeprop.ino:97:21: error: 'TIMEPROP_NUM_OUTPUTS' was not declared in this scope
static int relayNos[TIMEPROP_NUM_OUTPUTS] = {TIMEPROP_RELAYS};
^
/tasmota/tasmota/xdrv_48_timeprop.ino:97:46: error: 'TIMEPROP_RELAYS' was not declared in this scope
static int relayNos[TIMEPROP_NUM_OUTPUTS] = {TIMEPROP_RELAYS};
^
/tasmota/tasmota/xdrv_48_timeprop.ino: In function 'bool TimepropCommand()':
/tasmota/tasmota/xdrv_48_timeprop.ino:209:9: error: 'timeprops' was not declared in this scope
timeprops[XdrvMailbox.index].setPower( atof(XdrvMailbox.data), Tprop.current_time_secs );
^
Compiling .pio/build/tasmota-sensors/lib512/ESP8266WiFi/ESP8266WiFiSTA.cpp.o
Compiling .pio/build/tasmota-sensors/lib512/ESP8266WiFi/ESP8266WiFiScan.cpp.o
*** [.pio/build/tasmota-sensors/src/tasmota.ino.cpp.o] Error 1
===================================== [FAILED] Took 17.27 seconds =====================================

Environment Status Duration
tasmota-sensors FAILED 00:00:17.272
================================ 1 failed, 0 succeeded in 00:00:17.272 ================================
All done! Find your builds in Tasmota/build_output/firmware/

---

what am i doing wrong ?

[techman83]

@karlkashofer - I suggest taking this to discord or discussions rather than the merged PR. I am using the code in 2 projects, so it does work and will comment on a discussion thread if opened.

[karlkashofer]

Done: #11744 (comment)
Thanks,
KK