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

Add DB-Movas Provider #614

Closed
wants to merge 10 commits into from
Closed

Conversation

traines-source
Copy link
Contributor

@traines-source traines-source commented Dec 20, 2024

As discussed in #610, I have now implemented the DB Vendo/Movas API.
The tests are green and I have manually tested with Oeffi, everything looks fine, but more testing surely can't harm.
Some notes:

  • The nearby endpoint doesn't allow for POI search, but that doesn't seem to be surfaced in Oeffi anyways
  • The departure boards are limited to one hour, but I think this is OK
  • More messages/remarks/notices seem to be shown than before, although I'm only surfacing the important ones (usually disruptions, but also static ones like phone numbers for on demand buses etc. With the legacy DB Provider, some "Rufbus" were not distinguishable from normal buses, e.g. from Rothenburg o.d.T. to Dombühl at 20:31).
  • The cheapest available adult price is shown.
  • Error handling is not (yet) very granular

I noticed that Oeffi always gulps the last departure on the departure boards, because in StationDetailsActivity.java:438 it should be selectedDepartures.size()+1 due to position 0 being a dummy position. This becomes much more obvious due to the one hour limitation of the new API (and at this time of the day :).

Let me know if you also want a PR for Oeffi, however I've only quickly added the new provider and one would need to decide whether one wants to completely replace the old DB Provider (ironically, right now, the HAFAS API seems to work beautifully again, let's see for how long...)

@schildbach
Copy link
Owner

Huge thanks for this PR!

The nearby endpoint doesn't allow for POI search, but that doesn't seem to be surfaced in Oeffi anyways

That's right. "Öffi Stations" is only about stations/stops, not POIs or street addresses.

The departure boards are limited to one hour, but I think this is OK

Is that a limit from the API? I think 1 hour is very short, given that delays can easily be more than one hour. But of course it's better than nothing. Maybe in future we can ask DB to extend it.

More messages/remarks/notices seem to be shown than before, although I'm only surfacing the important ones

The old messages/categories were badly curated (by me, ages ago), so feel free to apply whatever filter feels right. We don't need to copy old behaviour here.

The cheapest available adult price is shown.

Cool! We never had prices with DB, afair.

Error handling is not (yet) very granular

Yes, that's expected. There is never any API doc about errors, so we need to fine-tune them over the coming weeks and months.

Let me know if you also want a PR for Oeffi

Sure, feel free! But I can also look after this bug if you prefer.

one would need to decide whether one wants to completely replace the old DB Provider

This is a very important decision indeed. From my gut feeling, I'd like to stay with the old provider as long as it works reasonably reliable. The HCI implementation has proven to be pretty solid over the years.

We could maybe provide the Movas API as an alpha alongside HCI, to help testing? If the HCI is switched off permanently, we can migrate everything to just one "DB" network ID. Related: Do you know if stations IDs have changed between the APIs? If so, have you witnessed any pattern, like adding a fixed constant?

I should have brought this up earlier, it's maybe too late now: Currently the PTE uses the old (and probably deprecated?) Android JSON API (org.json.JSONObject). At some point – and we could have started with this impl – I think it would make sense to migrate to something more modern. I always had Moshi in mind, because it is built on OkIo. But I'm not sure if it's still the right choice – we also have access to almost all of JDK 8 API nowadays, and in maybe 2 years JDK 11+.

@traines-source
Copy link
Contributor Author

traines-source commented Dec 21, 2024

Is that a limit from the API? I think 1 hour is very short

At least I haven't yet found out a parameter with which to influence it. One could of course do multiple requests under certain conditions (such as not enough departures returned), but there is not even a laterRef or something, DB Navigator just requests for one hour later if you click on "later" 🙈

Maybe in future we can ask DB to extend it.

Maybe you have better contacts into DB, from my experience they won't do something like that. On a side note, I haven't asked for permission so far to use the API, but at least it's without any API Key this time. Or one could switch to another DB API for the boards that support longer durations, but which have other drawbacks:

  • RIS::Stations API, needs an API Key
  • bahnhof.de API, which only allows querying for the current datetime (which would be fine for Oeffi I guess)
  • regio-guide.de API, which doesn't surface cancelled trips (which is currently also the behavior of Oeffi/DB Provider) and no messages/notices either

Which is why already for the sake of consistency I had opted for the API that DB Navigator uses for both routing and boards. And I personally would propose to show cancelled trips in the departure board, even if there is no cancelled flag in PTE for them, at least they now reliably show "Halt entfällt"/"Stop cancelled".

We could maybe provide the Movas API as an alpha alongside HCI, to help testing?

Sounds good, I'll make a PR for Oeffi then since that's basically what I've done. – EDIT: don't want to go through the hassle of creating a Gitlab account for these five lines, a git patch instead: dbmovas.patch.txt

Do you know if stations IDs have changed between the APIs?

It still uses the EVA numbers and in the queries the lid created out of that. But for POI and ADDRESS, PTE before used the complete lid as an id as far as I can tell. I can change it to also use that, then the serialized locations should be compatible I guess.

Android JSON API (org.json.JSONObject)

Indeed I've used it throughout and since it does not seem to be deprecated and it works nicely, I am a bit reluctant to change everything now :D But if you want I can have a look.

@traines-source
Copy link
Contributor Author

I think it has happened, DB HAFAS mgate.exe has been shut off for good, at least it now returns 503. If that is true, it probably doesn't make a lot of sense to keep the old DB Provider.

Copy link
Owner

@schildbach schildbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks really good overall.

src/de/schildbach/pte/util/HttpClient.java Outdated Show resolved Hide resolved
src/de/schildbach/pte/DbMovasProvider.java Outdated Show resolved Hide resolved
src/de/schildbach/pte/NetworkId.java Outdated Show resolved Hide resolved
@schildbach
Copy link
Owner

I just tested this against Öffi, and it looks very good. Sadly I didn't have addresses or POIs in my history, but stations are perfectly compatible.

@traines-source
Copy link
Contributor Author

Well, I guess we'll have to hope for the best with addresses then (but I think it should work, and it's a nice to have, not critical anyways IMHO).
I suppose you have then done the rebasing and will do the amendments to ResultHeader and comments regarding request headers yourself, otherwise lmk.

@schildbach
Copy link
Owner

schildbach commented Jan 10, 2025 via email

@traines-source
Copy link
Contributor Author

Ok done. Haven't tested with Oeffi again, but (new) tests are green, and I have only changed the NetworkId and minor stuff as discussed.

@schildbach
Copy link
Owner

Merged, and submitted a new version of Öffi to Google Play. I'll monitor exceptions.

Thanks again for your work!

@schildbach schildbach closed this Jan 11, 2025
@traines-source
Copy link
Contributor Author

Nice 👍 However, as far as I can see, you haven't fixed the selectedDepartures.size()+1 issue. As I said I think this is important as there are many stops where there is exactly one departure per hour, which will then not be shown (and even more confusing for the user, shown in the nearby view, but not when you open the departures view).
(And I don't see the update in Google Play yet but I suppose this will just take a couple of hours)

@schildbach
Copy link
Owner

It's live on the test channel already.

I'll look into the departures issue soon, I expect at least one quick follow-up release anyway. One particular thing I need to monitor is which devices will maybe choke on the Java streaming API (which wasn't used in PTE up to now).

@traines-source
Copy link
Contributor Author

Ah I didn't realize streams are such an issue, even though being on Java 8. Haven't done any Android development in a long time... Then we should just have replaced them with plain old Java I guess.

@schildbach
Copy link
Owner

Well let's see. It would be heartbreaking to see your streams code being converted to old loops.

@schildbach
Copy link
Owner

I just verified Android 8.1 works fine with streams. So I assume 8.0 does, too, because it's just a minor version bump between the two. So only Android 7/7.1 remains unknown for now.

@traines-source
Copy link
Contributor Author

Well according to the docs it should work starting API level 24, so Android 7.0 should be fine, right? I was rather worried about other projects that are using PTE and that maybe haven't yet upgraded to minSdk 24.
Another topic: In public-transport/db-vendo-client#10 it was discovered that the some rate limit is at 60 requests/minute. If it's IP-based, it shouldn't be a problem for Oeffi, but we couldn't yet exclude that it is solely based on the X-Correlation-ID (or some other) header, in which case all Oeffi users together might reach it (not sure how active your users are :D). In this case and if you push another release anyways, one could change this from "null" to randomly generated UUIDs, to better mimic the behavior of DB Navigator. What do you think?

@schildbach
Copy link
Owner

Does the API limit include station board requests? If yes, a single user could hit that limit if scrolling a lot.

Regarding X-Correlation-ID, sure, let's mimic DB Navigator. What format of UUIDs do they use?

@schildbach
Copy link
Owner

schildbach commented Jan 12, 2025

Answering my own Q: 0564dcdf-edbf-4412-a147-7299f6481470_64466773-556f-4aa8-b128-0948c4d60887

That looks like two version 4 UUIDs concatenated, separated by an underscore.

@schildbach
Copy link
Owner

@traines-source see 5522113

@traines-source
Copy link
Contributor Author

Jup thats exactly how marudor (and I in db-vendo-client now) does it, too 👍

@schildbach
Copy link
Owner

I found an incompatibility: java.time API isn't available until Android 8.

Would you like to have a look at this PR, which converts it to old-style DateFormat?

#621

@schildbach
Copy link
Owner

@traines-source Another Java 8-ism: #623

Would you like to have a look at the proposed replacement?

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.

4 participants