-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Linky : make use of DataConnect Enedis API #16355
base: main
Are you sure you want to change the base?
Conversation
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a first high level review of this work in progress.
*/ | ||
String formatAuthorizationUrl(String redirectUri); | ||
|
||
String[] getAllPrmId(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would not a List<String>
or Set<String>
be more convenient ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change in next commit
/** | ||
* @param listener Adds the given handler | ||
*/ | ||
public void setLinkyAccountHandler(LinkyAccountHandler accountHandler) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LinkyAccountHandler
could be passed by @Reference
in the constructor ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this class was remove from the new version after refactoring the code !
} | ||
|
||
@Reference | ||
protected void setHttpService(HttpService httpService) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same there
* The xxx manages the authorization with the Linky Web API. The servlet implements the | ||
* Authorization Code flow and saves the resulting refreshToken with the bridge. | ||
* | ||
* @author Gaël L'hopital - Initial contribution * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extraneous ' *'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix in next commit
*/ | ||
|
||
public class Customer { | ||
@SerializedName("customer_id") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding setFieldNamingPolicy(FieldNamingPolicy.LOWER_CASE_WITH_UNDERSCORES)
could save you from using @SerializedName
in many places. Also these classes could become records
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gael,
I finally remove the setFieldNamingPolicy, because it was conflicting with the deserialization use on the old Web API.
*/ | ||
|
||
public class IntervalReading { | ||
@SerializedName("value") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pointless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix in next commit
|
||
import java.util.HashMap; | ||
|
||
import org.eclipse.jetty.jaas.spi.UserInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove in next commit
*/ | ||
|
||
public class TempoResponse extends HashMap<String, String> { | ||
@java.io.Serial |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems to be need because i extend from HashMap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your IDE could generate the serial
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but what's @java.io.Serial
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've remove this, it seems to work without this stuff.
</div> | ||
|
||
</body> | ||
</html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing crlf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix in next commit
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/linky-communication-error/132889/39 |
Hello @lo92fr , are you still working on this ? |
It was mentioned somewhere (community forum ?) that API limit will be reached with only few users using this API. If confirmed, this looks like a wrong approach. PS: at the same time the current binding is now broken. |
MyElectricalData seems to be alive and kicking as a replacement (they display serving ~2500 PDL). The good thing going this way is watherver provider is choosen the binding will be aligned with Enedis API, then be freed of website evolution and brackages. |
Hello all, Sorry for my late response, but I was off home all February, with no access to my development computer. @clinique : I see your suggestion on pullrequest, I will address them as soon as possible. Some comments around the api limitation to 5 call / seconds.
So it's not perfect, but if every user is not calling the API at the same exact time, it should be ok. As someone already propose, I will also add some configuration so end user can change the endpoint from myelectricaldata to directly call Enedis API. I already put some code to handle the oauth configuration to do this. But It will need that the end user register officially to Enedis, and for this, the user will need to create a French law association. The is not a very complex process, but I think most user would not want to go this way ... Best regards, |
import org.eclipse.jdt.annotation.NonNullByDefault; | ||
|
||
/** | ||
* @author Gaël L'hopital - Initial contribution * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extraneous '*'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix in next commit
|
||
/** | ||
* | ||
* @author Gaël L'hopital - Initial contribution * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extraneous '*'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix in next commit
Ideally, we should keep an option for the users to choose between the original way with a direct connection to Enedis or the new way with a third-party. |
Hello @lolodomo, Yes, you're right. Laurent. |
Hello all, For information, I finalize to get a contract with Enedis last week. Laurent. |
I added the tag "work in progress" as "WIP" is still in the PR title. |
Hello All, I have a doubt about the Enedis direct implementations. But the matter is that Enedis think it as a central cloud service, not one deployed localy on every openhab user web server. |
Hello Clinique, I made a few commit this week, reviewing your comment. It seems to work ok, but still need to verify if Enedis will accept it to go in production this way, or if we need to have an intermediate platform to handle the consent / customer journey part. Also, about TimeSeries, it seems great, but I prefer introduce this latter if it's ok for you. If you have time, can you review the fixes so we can close the open conversations ? Thanks, |
public static final String YEAR_MINUS_1 = "yearly#lastYear"; | ||
public static final String YEAR_MINUS_2 = "yearly#year-2"; | ||
|
||
public static final String MAIN_IDENTITY = "main#Identity"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
main#identity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix on next commit
*/ | ||
public static final String LINKY_SCOPES = Stream.of("am_application_scope", "default") | ||
.collect(Collectors.joining(" ")); | ||
// "r:devices:*", "w:devices:*", "x:devices:*", "r:hubs:*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments will have to be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix on next commit
*/ | ||
|
||
public class TempoResponse extends HashMap<String, String> { | ||
@java.io.Serial |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your IDE could generate the serial
Hello Isiepel, This is still work in progress. The first provider MyElectricaldata is working quite well, just needing some few more test, and some code cleanup. The second provider is direct Enedis ApiConnection. This can't be use like this because it will need everyone to register to Enedis, that would not be possible easily. Other options will be to provide an alternative gateway like MyElectricaldata. But so far, I don't investigate these options. Possibly, if we want to integrate this in next version, the easiest way to go will be to drop the direct Enedis bridge, and only keep the MyElectricaldata data bridge. This is how it works today on the home assistant implementation. Laurent. |
Wdyt @clinique ? |
Yes, I think you should drop the Enedis part as it will more than likely not be used by anybody here. |
Signed-off-by: Laurent ARNAL <[email protected]>
Signed-off-by: Laurent ARNAL <[email protected]>
Signed-off-by: Laurent ARNAL <[email protected]>
Signed-off-by: Laurent ARNAL <[email protected]>
Signed-off-by: Laurent ARNAL <[email protected]>
Signed-off-by: Laurent ARNAL <[email protected]>
…es/MPS172TIB/prms Signed-off-by: Laurent ARNAL <[email protected]>
… same account Signed-off-by: Laurent ARNAL <[email protected]>
Signed-off-by: Laurent ARNAL <[email protected]>
…onment Signed-off-by: Laurent ARNAL <[email protected]>
Signed-off-by: Laurent ARNAL <[email protected]>
Signed-off-by: Laurent ARNAL <[email protected]>
Signed-off-by: Laurent ARNAL <[email protected]>
Signed-off-by: Laurent ARNAL <[email protected]>
… current data/time Signed-off-by: Laurent ARNAL <[email protected]>
Signed-off-by: Laurent ARNAL <[email protected]>
Signed-off-by: Laurent ARNAL <[email protected]>
Signed-off-by: Laurent ARNAL <[email protected]>
Signed-off-by: Laurent ARNAL <[email protected]>
Signed-off-by: Laurent ARNAL <[email protected]>
Signed-off-by: Laurent ARNAL <[email protected]>
Signed-off-by: Laurent ARNAL <[email protected]>
Signed-off-by: Laurent ARNAL <[email protected]>
review handling of URL for api based bridge Signed-off-by: Laurent ARNAL <[email protected]>
Signed-off-by: Laurent ARNAL <[email protected]>
…ount - add retry mechanisms to reconnect web bridge in case of errors Signed-off-by: Laurent ARNAL <[email protected]>
Signed-off-by: Laurent ARNAL <[email protected]>
add some more info to docs Signed-off-by: Laurent ARNAL <[email protected]>
Linky binding enhancement to support new DataConnect Enedis API
As view last week with Gael, this is a rewrite of the linky binding that use the official DataConnect Enedis API.
The use of this API will provide simpler authentification for the end user, as well as new functionnality like more granular data.
Description
This new binding version should offer right now the same functionnality that the old one.
This is WIP , will need on my side :
To fix compilation warning and code violation.
Do more in depth testing.
Would like to add more functionnality like :
The evolution of linky binding was discuss as said with Gael in the following thread:
https://community.openhab.org/t/linky-use-the-api-not-the-http-webpage/129230