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

Pid branch to address issue #15 #16

Open
wants to merge 1 commit into
base: pid_branch
Choose a base branch
from

Conversation

marcvs
Copy link

@marcvs marcvs commented Jan 4, 2021

Description:

Related issue (if applicable): fixes #

Checklist:

  • The pull request is done against the latest dev branch
  • Only relevant files were touched
  • Only one feature/fix was added per PR.
  • The code change is tested and works on core 2.3.0, 2.4.2, 2.5.2, and pre-2.6
  • The code change pass travis tests. Your PR cannot be merged unless tests pass
  • I accept the CLA.

@colinl
Copy link
Owner

colinl commented Jan 4, 2021

That all looks ok, but I am not able to test it at the moment. Do you feel you have given it sufficient testing?

@marcvs
Copy link
Author

marcvs commented Jan 5, 2021

From experience I can hardly answer "yes" to this question in general. Here's what I can say:

  • It works as expected
  • It didn't crash in the last ~17h
  • I can still use the web interface fluently (despite a load of 19)
  • It runs Core/SDK Version 2_7_4_9/2.2.2-dev(38a443e)
  • I'm willing to support it, in case of problems.

@colinl
Copy link
Owner

colinl commented Jan 5, 2021

That's great. Have you been able to test setting parameters via MQTT? I don't think it is necessary to test them all, if you check a couple of them I will check the code changes carefully so the rest should be ok

@marcvs
Copy link
Author

marcvs commented Jan 5, 2021

I'm only setting them via MQTT. That worked fine.

@colinl
Copy link
Owner

colinl commented Jan 5, 2021

OK, are you happy for me to merge it now or is there anything else you want to do?

@marcvs
Copy link
Author

marcvs commented Jan 5, 2021

I think it's fine.

I'll then also send the PR to arendst/development, as @ascillato suggested, right?

@ascillato
Copy link

I'll then also send the PR to arendst/development, as @ascillato suggested, right?

Great, Thanks 👍

@marcvs marcvs mentioned this pull request Jan 5, 2021
6 tasks
@marcvs
Copy link
Author

marcvs commented Jan 5, 2021

For the record: the PR against arendst/Tasmota is here: arendst#10412

@colinl
Copy link
Owner

colinl commented Jan 5, 2021

I understand the confusion early on in the issue now, I didn't realise that the pid branch had been incorporated into the arendst code. When I initially attempted to get it merged it was ignored, if I remember correctly.

@marcvs
Copy link
Author

marcvs commented Jan 6, 2021

I've had some discussion with Theo in the other PR, that I've submitted.

I was not really sure about the Timeprop_Command in xdrv_91_timeprop.ino (

boolean Timeprop_Command()
)

Functionality remained in place when I removed it. It looks like it was meant to receive commands for directly altering timeprop functionality. What shall I do about it?

xdrv_92_pid.ino contains two lines to publish PID power:

snprintf_P(mqtt_data, sizeof(mqtt_data), PSTR("{\"%s\":\"%s\"}"), "power", buf);

I've barred that behind an ifdef for backwards compatibility, as I've now managed to place the PID information into one JSON inside the /SENSOR topic (as I wanted to do years ago, but eventually had to face that I had no time).

It probably be easier to discuss one sole line of patches in the other PR, right?
arendst#10412

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

Successfully merging this pull request may close these issues.

3 participants