[evnotify] Initial contribution: New binding for evnotify online service #11537
Conversation
1a44cf6 to
23ed953
Compare
|
The issue with the build is probably that your project is still missing from |
|
@wborn What are the next steps to get the binding merged? Only the single pending review? How long does it usually take? |
|
The next step is that your PR will get reviewed and when a maintainer approves the changes the add-on is merged. 🚀 How long this all takes depends on how much code there is to review, season/holidays and how quickly review comments are processed. But from a quick scroll through your code it already looks quite good. 👍 With only 2 KLOC including DTOs and tests it should be relatively easy and fun to review. 😉 |
@wborn But when? |
| | `rapid_charge_port` | Switch | ON if connected to a rapid charging port | | ||
| | `normal_charge_port` | Switch | ON if connected to a normal charging port | | ||
| | `slow_charge_port` | Switch | ON if connected to a slow charging port | |
There was a problem hiding this comment.
Suggestion (not a request for change): you could have defined one unique channel for the type of charging port (rapid, normal, slow).
English detail: I am not sure "rapid" is appropriate. I would have naturally used "fast" but maybe "rapid" is correct too, I don't know.
There was a problem hiding this comment.
English detail: I am not sure "rapid" is appropriate. I would have naturally used "fast" but maybe "rapid" is correct too, I don't know.
You are right, but I just reused the terminology, of the ev notify service.
| | `aux_battery_voltage` | Number | Voltage of the aux battery | | ||
| | `dc_battery_voltage` | Number | Voltage of the DC battery | | ||
| | `dc_battery_current` | Number | Current of the DC battery | | ||
| | `dc_battery_power` | Number | Power of the DC battery | | ||
| | `cumulative_energy_charged` | Number | Amount of energy that has been charged | | ||
| | `cumulative_energy_discharged` | Number | Amount of energy that has been discharged | | ||
| | `battery_min_temperature` | Number | The minimum temperature of the battery | | ||
| | `battery_max_temperature` | Number | The maximum temperature of the battery | | ||
| | `battery_inlet_temperature` | Number | The inlet temperature of the battery | | ||
| | `external_temperature` | Number | External temperature | |
There was a problem hiding this comment.
You could certainly rather used Number with the appropriate dimension in order to let the user define the unit he would like.
| | `battery_max_temperature` | Number | The maximum temperature of the battery | | ||
| | `battery_inlet_temperature` | Number | The inlet temperature of the battery | | ||
| | `external_temperature` | Number | External temperature | | ||
| | `last_extended` | String | Timestamp of the latest export | | |
There was a problem hiding this comment.
The channel name is a little strange and if the channel is a timestamps, the item type is not the good one.
There was a problem hiding this comment.
Same as above: Just reused terminology of the ev notify service.
| | ------------------------------ | --------- | ---------------------------------------------- | | ||
| | `soc_display` | Number | State of Charge (Display) | | ||
| | `soc_bms` | Number | State of Charge (BMS) | | ||
| | `last_soc` | String | Timestamp of the latest state of charge export | |
There was a problem hiding this comment.
If the channel is a timestamps, the item type is not the good one.
There was a problem hiding this comment.
Same as above: Just reused terminology of the ev notify service.
|
|
||
| ### evnotify.items | ||
|
|
||
| ```properties |
There was a problem hiding this comment.
You can suppress "properties".
| * | ||
| * @author Michael Schmidt - Initial contribution | ||
| */ | ||
| public class ExtendedChargingDataDTO { |
There was a problem hiding this comment.
Please add @NonNullByDefault
|
|
||
| public OffsetDateTime getLastExtended() { | ||
| return lastExtended == null ? null | ||
| : OffsetDateTime.from(Instant.ofEpochSecond(lastExtended).atZone(ZoneId.of("Europe/Berlin"))); |
There was a problem hiding this comment.
Same here with hardcoded timezone
| * | ||
| * @author Michael Schmidt - Initial contribution | ||
| */ | ||
| public class EVNotifyClientImpl implements EVNotifyClient { |
There was a problem hiding this comment.
Please add @NonNullByDefault
| var basicRequest = HttpRequest.newBuilder(URI.create(String.format(BASIC_API_URL_PATTERN, akey, token))) | ||
| .header("accept", "application/json").build(); |
There was a problem hiding this comment.
Can you use proper type rather than "var" everywhere ?
| BasicChargingDataDTO basicChargingDataDTO = new Gson().fromJson(basicResponse.body(), | ||
| BasicChargingDataDTO.class); | ||
|
|
||
| var extendedResponse = client.send(extendedRequest, HttpResponse.BodyHandlers.ofString()); | ||
| validateResponse(extendedResponse); | ||
| ExtendedChargingDataDTO extendedChargingData = new Gson().fromJson(extendedResponse.body(), | ||
| ExtendedChargingDataDTO.class); |
There was a problem hiding this comment.
Rather than instantiating the Gson object everytime, you could have one as a member of your class, instantiated in the constructor ?
|
@mischmidt83 : are you available to handle review comments ? I would like to know if I take time to do the second part of my review or not. |
@lolodomo: yes, I'm available and try answering your questions resp. fixing your suggestions. It would be nice if you can continue the review. |
Signed-off-by: Dan Cunningham <dan@digitaldan.com> Signed-off-by: Michael Schmidt <mi.schmidt.83@gmail.com>
Move remaining channel details to the respective channels to simplify and avoid mistakes/errors. Signed-off-by: Marcel Verpaalen <marcel@verpaalen.com> Signed-off-by: Michael Schmidt <mi.schmidt.83@gmail.com>
|
Unfortunately it looks like we cannot bring this PR to the finish line. After two years of inactivity i close this PR. If there is a need to restart, feel free to re-open or start a new PR. |
This binding can be used to read data for a given electric vehicle from the EVNotify online service (account). Please see https://evnotify.de.
Therefore the binding provides a vehicle thing that represents an electric vehicle and it allows the user to monitor different data from the vehicle (e.g. state of charge, state of health, etc.).