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

P027 - INA219 #321

Closed
Grovkillen opened this issue May 30, 2017 · 16 comments
Closed

P027 - INA219 #321

Grovkillen opened this issue May 30, 2017 · 16 comments
Labels
Type: Bug Considered a bug
Milestone

Comments

@Grovkillen
Copy link
Member

Grovkillen commented May 30, 2017

The plugin does not have any controller settings. Thus the values won't get published. See screenshot from latest dev10.

PS. Also, in general I think "Enable" should be the last option in the list since it is where I choose if I want to start the device or not, this happens right before I hit submit.
PICTURE

@Grovkillen
Copy link
Member Author

Double post of #311 ?

@psy0rz psy0rz added the Type: Bug Considered a bug label Jun 5, 2017
@psy0rz psy0rz added this to the 2.0.0 milestone Jun 5, 2017
@Grovkillen
Copy link
Member Author

Please see this discussion for more information: https://www.letscontrolit.com/forum/viewtopic.php?f=6&t=3175

We need to make the plugin more clear of what the different values mean. (Just internal documentation in the webgui)

@ghost
Copy link

ghost commented Jun 10, 2017

The plugin was designed to send a single value per task as Domoticz has no custom triple value devices.
Is it verified to still work with Domoticz?

@Grovkillen
Copy link
Member Author

It works if used rules, but no... only one IDX per value is accepted by Domoticz.

@psy0rz
Copy link
Member

psy0rz commented Jun 10, 2017

woops... @krikk ?

@krikk
Copy link
Contributor

krikk commented Jun 10, 2017

if a sensor has 3 values, we should send 3 values... we should not cripple the sender, just because the receiver does not accept it. and when i understand @Grovkillen right you can work around it with rules...

this should be fixed in Domoticz not in esp easy...

@krikk
Copy link
Contributor

krikk commented Jun 10, 2017

and perhaps we should document the needed workaround solution if possible... @Grovkillen can you do that? ...because i do not have knowledge about domoticz...

@ghost
Copy link

ghost commented Jun 10, 2017

Easy to state that it should be fixed in Domoticz, but it's not likely going to happen. Also note that it may well be that 90% of our community uses Domoticz. To maintain backwards compatibility it would be better to have a selection box on what value(s) to report. (current,voltage,power,all)
Look at the P003 plugin to see how it was done there.

Remember that ESP Easy was build to make things easy (for the end user)...

@krikk
Copy link
Contributor

krikk commented Jun 10, 2017

i would prefer a more generic solution, like the ability to select which values are sent to the controller via a checkbox int the values list...

@ghost
Copy link

ghost commented Jun 10, 2017

That may even be better, although harder to implement as we currently have this SENSOR_TYPE_ mechanisme. This was build when we only supported Domoticz and Domoticz uses an update api with different syntax for different sensor types.

Being able to select individual values to report, you'd have to figure out in code how to translate this to the proper SENSOR_TYPE.

Would it be wise to build this as a generic device setting on all devices? I think it affects all device and controller plugin code, as well as some core code...

@Grovkillen
Copy link
Member Author

So what do we say, should I document it in the rules section? It might be good to know even if it should not be an official workaround?

@ghost
Copy link

ghost commented Jun 10, 2017

I'd prefer to restore the original selection box and just add the possibility to report "All". Looks the most simple and convenient solution, at least for short term (so all devs can focus on getting a stable release)

General recommendation: If changes break existing functionality, try to make the change an optional feature using a dropdown config or tick box. We have always made quite some effort to remain backwards compatible with older releases so we do not loose existing happy campers...

We should try to avoid using rules to workaround things that we change, as it makes things complicated for regular users.

@Grovkillen
Copy link
Member Author

I made a tutorial anyways since it might be good to be able to create custom publishing to Domoticz: https://www.letscontrolit.com/wiki/index.php/Tutorial_Rules#Custom_reports_to_Domoticz_with_own_IDX

@psy0rz
Copy link
Member

psy0rz commented Jun 11, 2017

what if we make the domoticz plugin just send the first value of an triplevalue to domoticz?

i agree with martinus that wd should stay backwards compatible for now.

@Grovkillen
Copy link
Member Author

That's the winner suggestion in my opinion. But will that mean that only Volt is possible for the INA219 plugin or is it possible to tell which one that is the first value?

@krikk
Copy link
Contributor

krikk commented Jun 11, 2017

@Grovkillen nailed the problem, thats the reason i implemeted the selection box see... see pull request...

hope this works

@psy0rz psy0rz closed this as completed Jun 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Considered a bug
Projects
None yet
Development

No branches or pull requests

3 participants