[linky] Add support for reading index instead of basic value

[lo92fr]

Support for reading index instead of basic value,

Description

This PR is about to get consumption index from the api instead of consumption value.
With consumption index, we will be able to dispatch the consumption to a specific tarif, and have graph where you can see consumption of different time area in the day.

For exemple, it will support tarif like:
Heure pleine / Heure creuse.
Tempo tariff with 6 dispatch cadran : Red, White, Blue & HP/HC.

And the first version only implement EnedisWeb gateway.

[lo92fr]

For exemple, you will obtains graph like this one:

[lo92fr]

Hello @lsiepel,

Can I ask you to relaunch a Copilot review on this PR.
There is many code change / refactoring since previous Copilot review, and so It will be more easy to start from a fresh analysis.

Also, I think the PR is now ready for final review.
I've made bunch of test this week, and everything seems to work no well.

@lolodomo, @clinique: fill free to test on your side, and make any comment if need.

Laurent

[Copilot]

Pull Request Overview

Copilot reviewed 20 out of 21 changed files in this pull request and generated 12 comments.

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[lo92fr]

Thanks @lsiepel,

I fixed the errors from the last copilot review.

Laurent.

[lsiepel]

Is this still WIP? If not, please remove the label.

[lo92fr]

Is this still WIP? If not, please remove the label.

No, this is ready for merge.
I don't think I have permission to remove the tag.
Can you do it for me ?

Thanks

[lolodomo]

Review part 1 of 2, all except README and class ThingLinkyRemoteHandler

[lolodomo]

Review of README

[lo92fr]

Hello @lolodomo,

Thanks for the review.
I've pushed the fixes.

[lolodomo]

Review changes in class ThingLinkyRemoteHandler until line 902

[lolodomo]

Please remove the parameters or comment them fully.
Same for other methods in that class.

[lo92fr]

done

[lolodomo]

If you provide Javadoc, you have to do it fully, that means mentioning all parameters and describe them.
You have now added a description to parameters but you only mentioned some of them and not all.

So either provide all parameters (with a description) or provider no parameters at all.

[lo92fr]

Ok, I've completed all existing javadoc with all parameters

[lolodomo]

Please move this test before line 550.

[lo92fr]

done

[lolodomo]

You did not move it as expected. My idea was to do this test even before building the channel.

[lo92fr]

Ok, this make sense. So in this order:

private void addChannel(List channels, ChannelTypeUID chanTypeUid, String channelGroup, String channelName,
String channelLabel, @nullable String channelDesc) {
ChannelUID channelUid = new ChannelUID(this.getThing().getUID(), channelGroup, channelName);

    if (getThing().getChannel(channelUid) != null) {
        return;
    }

    ChannelBuilder builder = ChannelBuilder.create(channelUid).withType(chanTypeUid).withLabel(channelLabel);

    if (channelDesc != null) {
        builder = builder.withDescription(channelDesc);
    }

    Channel channel = builder.build();

    if (channels.contains(channel)) {
        return;
    }

    channels.add(channel);
}

[lolodomo]

Yes, that was my suggestion.

[lo92fr]

Hello @lolodomo,

I've pushed the fixes for your review of this morning.
Sorry for all the typo in the comments of ThingLinkyRemoteHandler, I've check the full file, and will try to better check next time.
You will have to review my comment about label for dynamic channel, we can eventually prefix the label with the group name.
But not sure it is very a good thing as channel label are in french (coming from the data), and group are in English.

Laurent.

[lolodomo]

You will have to review my comment about label for dynamic channel, we can eventually prefix the label with the group name.
But not sure it is very a good thing as channel label are in french (coming from the data), and group are in English.

Yes, you're probably right. I have commented as a response to the original comment.

I would prefer to avoid the ERROR log level for the unexpected index mode.
Edit: ERROR level is already (wrongly at my opinion) used in this class, so this is just one more. So I don't know if it has to be changed or not.

Look at my 3 comments that are not yet marked as resolved.
I will finish the review today (last part of changes in class ThingLinkyRemoteHandler).

[lolodomo]

Previously, it was checked that the last value is not NaN.
You changed that to only check if the value is not null.
Is it because the API changed and NaN is no more happening ?

[lolodomo]

If I correctly remember, the API returns NaN for the consumption for yesterday in the early hours of the day. We detected that to know that the currently retrieved data is not yet ready and that we will have to retry later in the day.

[lo92fr]

Same there, I'm not totally sure of the reason of this change.
It's perhaps related to the previous comment.
I will need more time to check it, and be sure I don't brake anything.

[lolodomo]

I am not sure of the impact of this change.
If I correctly remember, returning null was intentional and was a trigger that we don't have the data yet.
Can you explain please.

[lolodomo]

If I correctly remember, returning null when data is missing was to not store any data in the cache and then to be sure that the API will be requested again the next hour.

[lo92fr]

I've remembered have introduce this because data for new index are never available on last day.
Always have a one day difference for this index,

I've also remembered have ask Enedis team about this behavior, but I far as I remember, have no response from there so far.

And yes, previously returning null was a way to be sure to re-request the data in next hours to be sure to have them.
So I definitively need to do some more check on this point to be sure that I've not broke something

[lolodomo]

Why not using a different method for indexes?

[lo92fr]

Yes perhaps a solution, or handle the return value differently base on the face we are handling index or consumption value

[lo92fr]

Hello @lolodomo,

I've finally revert this change.
I need it at the first place because the entry for meterReading array have 1095 entries, but when reading index we have only 1094 entries, and so the last value of the array was always empty when reading index.

But at the same time, I've done some modification in MeterReading on commit (925585e / review data array bound when using index).
This modification allocate only 1094 entries in the array when we are in index mode.
So with this modification, isDataLastDayAvailable return correctly even in IndexMode.

So the hack on ThingLinlyRemoteHandler don't have any reason to persist !

I've done some test, and everything works ok like this.

Laurent

[lo92fr]

You will have to review my comment about label for dynamic channel, we can eventually prefix the label with the group name.
But not sure it is very a good thing as channel label are in french (coming from the data), and group are in English.

Yes, you're probably right. I have commented as a response to the original comment.

I would prefer to avoid the ERROR log level for the unexpected index mode. Edit: ERROR level is already (wrongly at my opinion) used in this class, so this is just one more. So I don't know if it has to be changed or not.

Look at my 3 comments that are not yet marked as resolved. I will finish the review today (last part of changes in class ThingLinkyRemoteHandler).

I've finally remove the prefix on channel Label.
This will end up with something like this:

About the Error log level, not sure to understand what you want ?
As said, this condition it not really like to occur.
So what will be the good way to handle it:

• Return silently without logging an error (not sure that is a good thing).
• Change log level to something else that error.
• throw an exception, and toggle the thing to offline indicating an internal error ?
  Let me know.

About the 3 last comments, i'm currently looking at them.
Can take a little time as I've will have to check what happen in the different cases.

Laurent.

[lo92fr]

@lolodomo,

I've push the changed about this morning review.
For the 3 comments about the null return and the NaN check, have to said that I'm not currently totally sure it's safe.
I will make more review on it, and will come back later this week we a more detailed answer.

Laurent

[lo92fr]

Hello @lolodomo,

See my comment about the two remaining point.
I revert the code to the old one.

Laurent.