-
Notifications
You must be signed in to change notification settings - Fork 214
GTFS-rt : wheelchair access update #87
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
Closed
Closed
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is this really required? Why not keep it simple with NO_VALUE as UNKNOWN?
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.
For example, if a static GTFS says trip A is wheelchair accessible, how can a trip_update changes the value to Unknown? If we drop
NO_VALUEwe won't support this case. Maybe this is an edge case and in the eventuality this happens the producer can putWHEELCHAIR_INACCESSIBLEinstead. I don't feel too strongly on either choices.Uh oh!
There was an error while loading. Please reload this page.
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 definitely confusing at first glance, but the underlying issue is that protobufs don't allow enums to have a
nullvalue. I believe this is the first enumeration in GTFS that we're overriding in GTFS-rt, and as a result we haven't had this issue before.From https://developers.google.com/protocol-buffers/docs/javatutorial?csw=1 (emphasis mine):
I think it's important to support the case where the GTFS says that the trip is wheelchair accessible, but then real-time information indicates that this is unknown for some reason (
UNKNOWN) - for example, let's say an accessible vehicle was scheduled to run a trip, but then it broke down and was replaced with another vehicle, and dispatch doesn't know if that vehicle is wheelchair accessible. However, it's also important to say that there is no real-time information, and therefore the GTFS value should be used (NO_VALUE), especially for backwards compatibility and GTFS-rt feeds that aren't providing this field.@gcamp The proto3 spec says:
We're currently using
proto2for GTFS-realtime, but I'm assuming we should plan for eventually moving toproto3. This means thatNO_VALUEwould need to be0, which makes this even more confusing, as the enum values in GTFS-rt wouldn't match GTFS.I'm definitely supportive of trying simpler iterative implementations first especially if we have willing producers for them, but in this case because of the above I'm wondering if we should look at a simplified version of the carriage proposal instead.
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.
@barbeau Good point for proto3. We could use values from 0 to 4. Not the same numerical value compared the static GTFS, but since protobuf libraries would use the name of the value anyway I'm not sure this is a problem.
Would the carriage proposal fix those issues? The WheelchairAccessible value there doesn't have a NO_DATA and I don't think there's a way to not specify WheelchairAccessible value for a carriage if you want to specify other information.
Uh oh!
There was an error while loading. Please reload this page.
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.
Good point - in code I assume most/all protobuf libraries producing and consuming libraries would reference the enumeration name, not the ID, so maybe that's not too big of a stumbling block in software development, just confusing to read in the spec itself.
Yeah, thinking more, it doesn't. I was thinking that because
CarriageDescriptoris new, we wouldn't have the overriding issue that we would with GTFStrip.wheelchair_accessible. But, we'd still have to specify some behavior for interaction between the two. Unless we makeCarriageDescriptor.WheelchairAccessiblerequired, it could be empty, in which case we have the same overriding behavior problem as the default would be chosen.So in summary using values from 0 to 4 for
TripUpdate.WheelchairAccessibleenumeration seems like a reasonable approach.Given some of the potential interaction issues with GTFS here, I'd like to see a working implementation before we vote on it. Having a producer and consumer is a requirement for calling a GTFS vote for new fields, but in the past those requirements have been relaxed for GTFS-rt given the potential cost of an agency reverting a deployed protobuf extension to the canonical protobuf after the field is approved. Instead, a field has been voted on an accepted as "experimental" (see occupancy proposal). Due to the interactions with GTFS, though, this proposal seems potentially more complicated to me.