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

Added initial support to updated timetables for trips/[tripId]/stoptimes transit index call. #1495

Conversation

vreixo
Copy link
Contributor

@vreixo vreixo commented Sep 11, 2014

This is the code being used for A Coruña server, and might be useful for other servers too, meanwhile all the class is not fully connected to timetableSnapshot.

With this, for API endpoint trips/[tripId]/stoptimes, realtime values are returned.

@hannesj
Copy link
Contributor

hannesj commented Mar 4, 2015

This is already fixed in master, and should probably be closed.

@abyrd
Copy link
Member

abyrd commented Mar 4, 2015

Thanks for checking that @hannesj

@abyrd abyrd closed this Mar 4, 2015
@sdjacobs
Copy link
Contributor

sdjacobs commented May 9, 2016

@hannesj and @abyrd, could you take a look at this again? Based on my understanding and testing of the code, the API call /trips/{tripId}/stoptime currently returns scheduled stoptimes - though please correct me if I'm wrong here! This PR would enable that call to return stoptimes that are updated from realtime data.

@abyrd
Copy link
Member

abyrd commented Jun 15, 2016

@sdjacobs and @barbeau thanks for drawing my attention to this. Looking at the source code, master does seem to only look at the scheduled timetable. @hannesj can you explain what you meant when you said this was fixed in master? Are you using a different endpoint to get the realtime information?

@vreixo sorry I closed this without further investigation, I thought we had confirmation that someone had already integrated the feature.

However, in this PR the scheduled timetable is simply swapped out for a real-time timetable. It seems to me that both should be included in the response.

@barbeau
Copy link
Contributor

barbeau commented Jun 15, 2016

However, in this PR the scheduled timetable is simply swapped out for a real-time timetable. It seems to me that both should be included in the response.

+1 for this, ideally we would have both

@abyrd
Copy link
Member

abyrd commented Jun 16, 2016

Actually I was wrong, the TripTimeShort constructor extracts both the scheduled and real-time arrivals and departures, as well as the delays. @vreixo's method is a good one for getting the right timetable. I'm merging this.

@abyrd abyrd reopened this Jun 16, 2016
abyrd added a commit that referenced this pull request Jun 16, 2016
…ithub.com/vreixo/OpenTripPlanner into vreixo-master_realtime_trips_stoptimes_tametable

Also renamed a method to make it more clear.
Fixes #1495
@abyrd abyrd closed this Jun 16, 2016
@abyrd
Copy link
Member

abyrd commented Jun 16, 2016

Hi @vreixo, if you get a chance could you explain what you meant by "meanwhile all the class is not fully connected to timetableSnapshot"?

@hannesj
Copy link
Contributor

hannesj commented Jun 16, 2016

@abyrd Sorry, I must have overseen something. I don't remember why I responded like that.

This patch has an issue if no StoptimeUpdater is configured, in that case req.rctx.timetableSnapshot is null and this will throw an NPE. Also this will always show the realtime departures of the current date, which I'm not sure is a wanted behaviour.

@hannesj
Copy link
Contributor

hannesj commented Jun 16, 2016

A better way would be using TripPattern#getUpdatedTimetable to fetch the updated timetable.

@barbeau
Copy link
Contributor

barbeau commented Jun 16, 2016

Thanks @abyrd!

@abyrd
Copy link
Member

abyrd commented Jun 16, 2016

@hannesj thanks for the feedback. This will indeed always show the current date, but since this call has no date parameter this seems like the obvious default. And thanks for pointing out the null check in getUpdatedTimetable. It is important that this work for the vast majority of OTP deployments that do not have any real-time data. I will make that change.

@abyrd
Copy link
Member

abyrd commented Jun 16, 2016

So in the end I didn't use TripPattern#getUpdatedTimetable because that expects a ServiceDay instead of a ServiceDate. I just added a null check. @hannesj and @barbeau please look at / test this out and let me know if there are any remaining problems.

@barbeau
Copy link
Contributor

barbeau commented Jun 16, 2016

Thanks @abyrd! @sdjacobs Could you give this a shot with the OBA Android app? Latest commits @abyrd references are in master branch here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants