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

GPIOtoggle,x returns both json and plain text #2892

Closed
ironataerial opened this issue Feb 17, 2020 · 24 comments
Closed

GPIOtoggle,x returns both json and plain text #2892

ironataerial opened this issue Feb 17, 2020 · 24 comments
Labels
Category: Rules Related to the rule engine Type: Bug Considered a bug

Comments

@ironataerial
Copy link

ironataerial commented Feb 17, 2020

http://IP/control?cmd=GPIOtoggle,4

returns :

{
"log": "Toggle GPIO 4 Set to 1",
"plugin": 1,
"pin": 4,
"mode": "output",
"state": 1
}

Ok

Should be either JSON or OK but not both ?

@TD-er
Copy link
Member

TD-er commented Feb 17, 2020

Should be either JSON or OK but not both ?

Yep.
What build is this?

Edit:
Just tested it and it also occurs with the latest build I have on my test node here.

@giig1967g Is this taken care of in your PR?

@ironataerial Can you test this test build

@TD-er TD-er added Category: Rules Related to the rule engine Type: Bug Considered a bug labels Feb 17, 2020
@ironataerial
Copy link
Author

mega-20200204 is the release I noticed this behavior on
another unit on mega-20190805 behaves properly.

BTW this is not limited tot he GPIOtoglle as initially posted, as I just confirmed gpio,x,y pulse,x and longpulse,x

-D

@ironataerial
Copy link
Author

ironataerial commented Feb 17, 2020

@TD-er
the mega-20200204-33-PR_2778 returns only the text.
Used to return the JSON in the prior version.

-D

@TD-er
Copy link
Member

TD-er commented Feb 17, 2020

Thanks for testing/verifying.

@ironataerial
Copy link
Author

@TD-er
The JSON response is preferred well over the text as it used to be.

@ironataerial
Copy link
Author

The same (JSON and OK) remains in the mega-20200305
JSON only is what is preferred.

{
"log": "GPIO 4 Set to 1",
"plugin": 1,
"pin": 4,
"mode": "output",
"state": 1
}

Ok

Thank you
-D

@ironataerial ironataerial reopened this Mar 5, 2020
@TD-er
Copy link
Member

TD-er commented Mar 5, 2020

That's correct as it has not yet been fixed (or a fix merged)

giig1967g added a commit to giig1967g/ESPEasy that referenced this issue Mar 5, 2020
@AloisKlingler
Copy link

mega-20200204 is the release I noticed this behavior on
another unit on mega-20190805 behaves properly.

BTW this is not limited tot he GPIOtoglle as initially posted, as I just confirmed gpio,x,y pulse,x and longpulse,x

-D

thanks for mentioning a not affected release, after downgrading to mega-20190805 I can do parsing in node red without complications. :-)

@TD-er
Copy link
Member

TD-er commented Mar 19, 2020

OK, I've looked into the code and it seems some plugins do send the status themselves.
So this is a design flaw that was made apparent when I tried to remove duplicate code to make it more generic in handling the return values and status updates.

Some of these, the switch plugins and GPIO extenders, are now about to undergo a big change (see PR: #2778 ), but there are still others that will have the same issue:

  • P001 Switch
  • P009 MCP
  • P019 PCF8574
  • P011 PME
  • P022 PCA9685
  • P075 Nextion

The design flaw is that we don't properly detect if the status has been sent, but just assume it has by looking at the return value when calling the PLUGIN_WRITE and always return "\nOK"
The first 3 of these will be dealt with in the linked PR, but for the others we still have this issue.

Possible approaches to fix this:

  • Let the plugin set a (new) flag in the event when a status update was sent
  • Return the string to send as status update in the event.

@giig1967g
Copy link
Contributor

giig1967g commented Mar 20, 2020

this should be fixed in #2778, at least for P001, P009 and P019

@ironataerial
Copy link
Author

this should be fixed in #2778, at least for P001, P009 and P019

It does not seem to have any affect on 20200328 tested just now,

http:///control?cmd=gpio,4,1

results to :

{
"log": "GPIO 4 Set to 1",
"plugin": 1,
"pin": 4,
"mode": "output",
"state": 1
}

Ok

@TD-er
Copy link
Member

TD-er commented Mar 28, 2020

It does not seem to have any affect on 20200328 tested just now,

That pull request has (still) not yet been merged.

@ironataerial
Copy link
Author

It does not seem to have any affect on 20200410 tested just now,

Assume it has not been merged yet ?

@TD-er
Copy link
Member

TD-er commented Apr 11, 2020

It does not seem to have any affect on 20200410 tested just now,

Assume it has not been merged yet ?

That's correct.
It is included in the big GPIO PR and I didn't want to merge to many big changes at once.
So I will try to split that fix from that PR later this evening so it is no longer dependent.
After all it is a separate issue.

@giig1967g
Copy link
Contributor

@TD-er
Hi, if I remember correctly, the change involves the new internal command structure.
If I may suggest, I think that you should merge the whole PR instead of just part of it. Also because my code is based on November release and its becoming difficult to maintain.
Maybe you could have a look at it and finally merge it.
Thanks

@TD-er
Copy link
Member

TD-er commented Apr 11, 2020

@giig1967g Problem with the current state of that PR is that it will break existing Domoticz (and maybe others too) integration and I don't have a clue yet why.

@giig1967g
Copy link
Contributor

Hi,
Why don't you explain to me the problem?
With examples and cases.
I might help finding the cause.

@TD-er
Copy link
Member

TD-er commented Apr 12, 2020

Well I am not really sure your implementation is bad.
It may also be the way we did handle Domoticz stuff was the bad way after all.
It is truly unintuitive as it is done right now, but it is a fact it has been used a while so there are systems out there using exactly that way of switching. So it will be a breaking change.

Just about your remark of diverging from the main branch.
I did bring the pull request in sync with the main branch a few days ago, so you can make a branch from the current point of the pull request.
Not saying I should take much more time to look into it, but more like I don't want to complicate stuff for you.

@ironataerial
Copy link
Author

It does not seem to have any affect on 20200426 tested just now.

@TD-er
Copy link
Member

TD-er commented Apr 26, 2020

It is also not been merged/fixed jet.

TD-er pushed a commit to giig1967g/ESPEasy that referenced this issue Jun 10, 2020
TD-er pushed a commit to giig1967g/ESPEasy that referenced this issue Jul 3, 2020
@ironataerial
Copy link
Author

It does not seem to have any affect on 20200720 tested just now.

@giig1967g
Copy link
Contributor

unfortunately no.
The PR has not been merged yet.

TD-er had a lot of work fixing wifi issues.

Hopefully in the next release.

@TD-er
Copy link
Member

TD-er commented Jul 21, 2020

The next (building right now) also does not have a fix for this...

@ironataerial
Copy link
Author

Seems fixed on 20201022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Rules Related to the rule engine Type: Bug Considered a bug
Projects
None yet
Development

No branches or pull requests

4 participants