[SNCF] A binding to get French railways arrivals and departures

[clinique]

No description provided.

[lolodomo]

I will review your PR.

[lolodomo]

Review part 1 of 3

[lolodomo]

Review part 2 of 3

[lolodomo]

Remains for me the class StationHandler to review.

[lolodomo]

Review part 3 of 3

[lolodomo]

@clinique : I am going to test the binding.
What remains to do now:

1. enhance the example in documentation (if you are ok)
2. fix bridgeStatusChanged
3. consider what reports SAT (this is very easy to fix like replacing URL by url and cutting the catch exception)

[clinique]

Fine for me @lolodomo

[lolodomo]

Reading the FAQ, I see that for Transilien, thtis is unfortunately not real-time:

L'API propose les horaires planifiés et temps réel des trains SNCF suivants : TGV, Intercités, TER, Lyria et Eurostar. Les horaires des trains Transilien sont disponibles en planifiés uniquement. Pour avoir accès aux horaires des prochains passages des trains Transilien, rendez-vous sur data.sncf.com.

So probably something to mention in the documentation.

Edit: maybe you could consider data.sncf.com too ?

[lolodomo]

Doing my first tests, I see that the values for the channels code and name are identical. The values for channels commercial mode and network are identical too. Did you notice the same ? Do you thing it is really necessary to have all these channels ? Or maybe having some of them being advanced ?

In your discovery service, please call the superclass with false as a last parameter as there is no background discovery.

Regarding the discovery, 2 things with the same label are found, one is a bus station (property ending by ":Bus") and the other a train station (property ending by ":RapidTransit"). Maybe you could include the type of station in the label ?
Maybe "RapidTransit" is not the only type for a train station.

The refresh of data is not working at all for me.

[lolodomo]

The refresh of data is not working at all for me.

After adding few logs:

14:26:22.132 [DEBUG] [sncf.internal.handler.StationHandler] - wishedDelay is 157 seconds
14:26:22.132 [DEBUG] [sncf.internal.handler.StationHandler] - existingDelay is 0 seconds
14:26:22.132 [DEBUG] [sncf.internal.handler.StationHandler] - Do nothing, existingDelay earlier than wishedDelay

So the problem is your retrieval of the existing delay which is 0.
Maybe you have to reschedule when the existing delay is 0.

[clinique]

@clinique : I am going to test the binding. What remains to do now:

1. enhance the example in documentation (if you are ok)

2. fix `bridgeStatusChanged`

3. consider what reports SAT (this is very easy to fix like replacing `URL` by `url` and cutting the catch exception)

Done for the last two points

Reading the FAQ, I see that for Transilien, thtis is unfortunately not real-time:

L'API propose les horaires planifiés et temps réel des trains SNCF suivants : TGV, Intercités, TER, Lyria et Eurostar. Les
horaires des trains Transilien sont disponibles en planifiés uniquement. Pour avoir accès aux horaires des prochains
passages des trains Transilien, rendez-vous sur data.sncf.com.

So probably something to mention in the documentation.

Edit: maybe you could consider data.sncf.com too ?

On my test environment, I have Transilien scheduling working fine but I will add a mention in the README.MD.
I'm also going to take a look at data.sncf.com to see what could be of any benefit for the binding. This is the same open data structure that used in vigicrues and meteoalert so most of the work already exists.

Doing my first tests, I see that the values for the channels code and name are identical. The values for channels
commercial mode and network are identical too. Did you notice the same ? Do you thing it is really necessary
to have all these channels ? Or maybe having some of them being advanced ?

I agree, commercial mode and network seems to be the same. It's not always the case for code and name. I'm going to set two of them as advanced.

[lolodomo]

What annoys me with these calls called in parallel in separate threads is that you have to consider concurrent calls to scheduleRefresh and freeRefreshJob. I am not sure this is something you took into consideration, did you ?
Is it really necessary to do that ? Two calls in sequence could be ok, no ?

[clinique]

Well, I had to do this because I faced a concurrent exception at jetty level with the previous code organization. It is the only I found to solve the problem. You're right, I can test it in a single scheduler submit call.

[clinique]

Changed.

[lolodomo]

I still do not understand why you create a new thread while updateThing is already executed by a scheduled thread ?

[clinique]

If/when I don't do it in a thread, I get this error on second call :

17:32:17.180 [DEBUG] [f.internal.handler.SncfBridgeHandler] - Execution interrupted : null
java.lang.InterruptedException: null
	at java.util.concurrent.locks.AbstractQueuedSynchronizer.acquireSharedInterruptibly(AbstractQueuedSynchronizer.java:1343) ~[?:?]

[lolodomo]

I just saw a case while next arrival was at 19:45 and at 19:45 it changed to 19:46 (same train). So it would mean this is real-time.

[clinique]

@lolodomo : I think I've addressed your last comments / remarks. I'm going to push a new update.

[lolodomo]

@clinique, I am running your last version.
Unfortunately, I can confirm there is a major issue with the direction which is often wrong. Good timestamps and good line but wrong direction.
I tried in different train stations.
Maybe the wrong field is retrieved in the JSON ?

[lolodomo]

I found the solution, the direction has to be retrieved in display_information.
So this code is working after adding the field direction:

            case DIRECTION:
                // return fromNullableString(passage.route.direction.name);
                return fromNullableString(passage.displayInformations.direction);

[clinique]

I found the solution, the direction has to be retrieved in display_information. So this code is working after adding the field direction:

            case DIRECTION:
                // return fromNullableString(passage.route.direction.name);
                return fromNullableString(passage.displayInformations.direction);

Thanks for your bug hunting. Pushing the update.

[lolodomo]

@clinique : still one comment not fully resolved (about creating additional threads).

My tests are positive.

New general comments:

1. In case you have two trains at the same time (same minute), for example each in one direction, you will loose one of them. Maybe refreshing ONE minute after the planned arrival could be reduced to 10 seconds after the planned arrival to reduce this risk ? Another option would be to have channels for the two next trains. WDYT ?

2. Regarding default pattern for timestamps, I still think it should rather be only hours + minutes but it should round to minutes, that is the real value is 18:30:50 should be displayed as 18:31. I don't know if this is what is done with patterns. I compared the times with the ones provided by the SNCF Android app. But this could be improved later.

3. You did not find the train platform in the returned data (I did not see it myself but have only a quick look on data) ?

4. After my tests, I have the feeling that for Ile-de-France, the data are planned data, oot real-time data.

[lolodomo]

I forgot one point: now that route is no more used, unsued classes could be removed ?

[clinique]

@clinique : still one comment not fully resolved (about creating additional threads).

Just answered it a few minutes ago and retested (see above in the conversation thread).

My tests are positive.

New general comments:

1. In case you have two trains at the same time (same minute), for example each in one direction, you will loose one of them. Maybe refreshing ONE minute after the planned arrival could be reduced to 10 seconds after the planned arrival to reduce this risk ? Another option would be to have channels for the two next trains. WDYT ?

I'm going to reduce to 10 seconds and see if it does the job.

2. Regarding default pattern for timestamps, I still think it should rather be only hours + minutes but it should round to minutes, that is the real value is 18:30:50 should be displayed as 18:31. I compared to the the times provided by the SNCF Android app. It could be improved later.

I quite do not get the point of rounding a timestamp provided sharp. If used in automation, the sharper the better. Ok to come back on this on return of users experience.

3. You did not find the train platform in the returned data (I did not see it myself but have only a quick look on data) ?

Yes, I searched but did not find it neither.

4. After my tests, I have the feeling that for Ile-de-France, the data are planned data, oot real-time data.

I have a plan to take care of announced service disruptions (not time to dive on this currently), that could generate difference between planned and real-time data.

[lolodomo]

I quite do not get the point of rounding a timestamp provided sharp. If used in automation, the sharper the better. Ok to come back on this on return of users experience.

I don't want to round the raw data, only the default display value in UI to show only hours+minutes.

[clinique]

I quite do not get the point of rounding a timestamp provided sharp. If used in automation, the sharper the better. Ok to come back on this on return of users experience.

I don't want to round the raw data, only the default display value in UI to show only hours+minutes.

Don't you think it'll be weird for users to see a given time in logs and a different one in the interface ?

[lolodomo]

I quite do not get the point of rounding a timestamp provided sharp. If used in automation, the sharper the better. Ok to come back on this on return of users experience.

I don't want to round the raw data, only the default display value in UI to show only hours+minutes.

Don't you see it'll be weird for users to see a given time in logs and a different one in the interface ?

Yes, maybe.
But in SNCF app, they display hours+minutes. They clearly do not have a precision at the second ;)
But ok, keep it as it is.

[lolodomo]

LGTM now

[lolodomo]

@clinique: let me know if your test is positive and I will then merge. I am going to run my own test in parallel.

[clinique]

@clinique: let me know if your test is positive and I will then merge. I am going to run my own test in parallel.

Yes, you can merge. Thanks for your attentive and patient review !

[lolodomo]

Yes, it seems to work well for me too.
Thank you for your new binding contribution Gael.

[lolodomo]

@clinique: if not yet done, you could add your binding's logo to the openHAB website. See https://www.openhab.org/docs/developer/bindings/#add-your-binding-s-logo-to-the-openhab-website