fix: Define defaults for VehiclePosition occupancy percentage and status

[barbeau]

As discussed in #247, default values aren't currently defined for VehiclePosition.occupancy_percentage and VehiclePosition.occupancy_status.

This pull request adds defaults for VehiclePosition.occupancy_percentage and VehiclePosition.occupancy_status so erroneous defaults aren't read by GTFS-realtime consumers.

TODO:

• Test backwards compatibility for occupancy_status - Done for Java in MobilityData/gtfs-realtime-bindings@a4464a4 in branch https://github.com/MobilityData/gtfs-realtime-bindings/tree/occupancy-defaults-status. No changes in behavior when reading post default PBs with pre-default code.
• Test backwards compatibility for occupancy_percentage - what happens when a -1 value that was serialized by a new int32 producer is read by a legacy consumer still using the uint32 in the .proto. Done for Java in MobilityData/gtfs-realtime-bindings@48bd41a in branch https://github.com/MobilityData/gtfs-realtime-bindings/commits/occupancy-defaults-status-percentage. No changes in behavior when reading post default PBs with pre-default code, including reading -1 values for occupancy_percentage.
• Test reading -1 occupancy_percentage value on legacy C++ consumers

Fixes #247

[skinkie]

Could you elaborate why this optional value needs an initialisation? If I would parse this feed without the value present, it would be just "not available".

[barbeau]

@skinkie Could you read #247 and let me know if you still have questions? I explained it there.

[skinkie]

@skinkie Could you read #247 and let me know if you still have questions? I explained it there.

I have only active experience with C, Java and Python parsing, both obviously all support NULL, null and None, but that is not my point here. When I want to check for a stopTimeUpdate, I first have to ensure stopTimeUpdate.hasDeparture() for which languages are optional values in GTFS-RT not supporting this hasAttribute method?

[barbeau]

I first have to ensure stopTimeUpdate.hasDeparture(). for which languages are optional values in GTFS-RT not supporting this hasAttribute method?

Yes, that's the issue here. We can't guarantee that all protobuf consuming software include methods like hasX(), in which case you'd just need to read the value directly. And if you read the value directly for an optional value that doesn't have an explicit default set in the .proto, you get the "default default" defined in the protobuf docs.

Here's one example for C# - MobilityData/gtfs-realtime-bindings#52.

You can see this actually applies to other optional GTFS-realtime fields as well that we could fix, including bearing, speed, and vehicle stop status.

[barbeau]

I should also mention that this change will help developers new to protobufs that don't know they need to call hasX() before reading the value.

[skinkie]

I first have to ensure stopTimeUpdate.hasDeparture(). for which languages are optional values in GTFS-RT not supporting this hasAttribute method?

Yes, that's the issue here. We can't guarantee that all protobuf consuming software include methods like hasX(), in which case you'd just need to read the value directly. And if you read the value directly for an optional value that doesn't have an explicit default set in the .proto, you get the "default default" defined in the protobuf docs.

How is that binding handling a missing Departure or Arrival then technically (kind of the basics when using GTFS-RT)? I honestly think this should not be solved by default values, rather the binding generator for C# should be fixed.

[darylweinberg]

Just a note, Capital Metro Austin will begin publishing occupancy_status in about 2 weeks, for our entire bus fleet. We are only choosing to publish CRUSHED_STANDING_ROOM_ONLY, FULL and NOT_ACCEPTING_PASSENGERS. When one of these values is not present, we do not include occupancy_status for that vehicle. We have worked with our partners to display “Notice: Crowded Vehicle” if any of these three values are present. I will be reaching to other consumers to explain what we are doing and see if they want to provide the data that is available. Thank you, [http://www.capmetro.org/email/eSigLogo.png]<http://www.capmetro.org/> Daryl Weinberg Transit Systems Architect [http://www.capmetro.org/email/sig_fb.png]<https://www.facebook.com/capitalmetro?_rdr=p> [http://www.capmetro.org/email/sig_tw.png] <https://twitter.com/CapMetroATX> o: 512-369-6216 | e: Daryl.Weinberg@capmetro.org<mailto:Daryl.Weinberg@capmetro.org> | w: capmetro.org<http://www.capmetro.org/> From: Sean Barbeau <notifications@github.com> Sent: Thursday, September 24, 2020 9:51 AM To: google/transit <transit@noreply.github.com> Cc: Subscribed <subscribed@noreply.github.com> Subject: Re: [google/transit] fix: Define defaults for VehiclePosition occupancy percentage and status (#248) EXTERNAL E-MAIL @skinkie<https://github.com/skinkie> Could you read #247<#247> and let me know if you still have questions? I explained it there. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#248 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AFZNIRVRPDU6UUJOLPRFA6TSHNMGTANCNFSM4RYMMWNA>.

[jxeeno]

Yeah, I think I'm on the same page as @skinkie on this one. Even though it's not explicitly supported by proto2, it's implicitly a requirement for the bindings to support expressing optional values as null-like or have a hasX() in order for delay/time in GTFS-R to be interpreted in any meaningful way.

[barbeau]

@skinkie @jxeeno Thanks for the feedback. Could you elaborate on the downside to defining defaults in the gtfs-realtime.proto file? What do you see as the main drawbacks?

[gcamp]

I agree that generally there are workaround in the implementation to achieve the same behaviour. However since there's nothing in the spec that mention what is the actual default, we have a vendor that insist that they can not include the occupancy_status value if the occupancy is EMPTY (the first value of the enum which is the default if no default is specifically defined).

Adding a default value would clear that ambiguity.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This pull request has been closed due to inactivity. Pull requests can always be reopened after they have been closed. See the Specification Amendment Process.