WIP: [Android] Speed limit exceed check improvements #9167

strump · 2024-08-30T13:50:30Z

strump commented

2024-08-30 13:50:30 +00:00

Closes #8934.
Alternative implementation of #9150 .

What problem do I try to resolve?

Android UI (NavMenu class) needs to show current speed and speed limit in local units ("km/h" or "mi/h"). Current implementation uses app.organicmaps.util.StringUtils.nativeFormatSpeedAndUnits(speedMps) for current speed and for speed limit. These JNI calls could be slow when invoked on every location update (once per second). Also to indicate speedometer red when speed is greater than speed limit UI code should have speed values in "km/h" or "mi/h" units (another JNI call?).

What is done

Check speed limit exceed on the backend with respect to user locale. Speed is converted to km/h or mi/h depending on user settings. Localized speed is rounded to compare only integer parts.

Android Pass isSpeedLimitExceeded flag to RoutingInfo. Use this flag to mark speedometer red.

TODO: Add unit-test to cover routing::RoutingSession::IsSpeedLimitExceeded(...)

As far as I can see current GPS info with speed value is passed from Android to backend in the next way:

LocationState.nativeLocationUpdated(time, lat, lon, accuracy, altitude, speed, bearing) - wrappes values into location::GpsInfo instance and calls android::Framework
android::Framework::OnLocationUpdated() - passes GPS info to ::Framework
Framework::OnLocationUpdated() - passes GPS into to RoutingManager
RoutingManager::OnLocationUpdate() - passes GPS info to extrapolation::Extrapolator
extrapolation::Extrapolator::OnLocationUpdate() - saves GPS info into field m_lastGpsInfo.

And to get last known GPS location and speed I had to add methods:

map::Framework::GetLastLocation() which calls RoutingManager
RoutingManager::GetLastLocation() which calls extrapolation::Extrapolator
extrapolation::Extrapolator::GetLastLocation() which returns pointer to the field m_lastGpsInfo.

Is this OK?

Update 2024-09-05

Introduced new classes (name could be changed):

// C++
class SpeedFormatted
{
  double m_speed; // Speed in km/h or mile/h depending on m_units.
  measurement_utils::Units m_units; // Metric or imperial.
}

// Java
class SpeedFormatted
{
  public final double mSpeed; // Speed value in local units.
  public final String mSpeedStr; // Speed value formatted.
  public final SpeedFormatted.Units mUnits; // Two options "km/h" or "mi/h"
}

So instead of using Pair<String, String> StringUtils.nativeFormatSpeedAndUnits(double metersPerSecond) method to get formatted speed we pass both current speed and speed limit as SpeedFormatted objects from Framework to NavMenu. And now NavMenu class decides if there is speeding by comparing speed values in "km/h" or "mi/h" units.

TODO: Verify in the wild on a real road with speed limit.

TODO: Add unit-test to cover SpeedFormatted class.

TODO: check if iOS code need changes.

Closes #8934. Alternative implementation of #9150 . ## What problem do I try to resolve? Android UI (`NavMenu` class) needs to show current speed and speed limit in local units ("km/h" or "mi/h"). Current implementation uses `app.organicmaps.util.StringUtils.nativeFormatSpeedAndUnits(speedMps)` for current speed and for speed limit. These JNI calls could be slow when invoked on every location update (once per second). Also to indicate speedometer red when speed is greater than speed limit UI code should have speed values in "km/h" or "mi/h" units (another JNI call?). ## What is done Check speed limit exceed on the backend with respect to user locale. Speed is converted to km/h or mi/h depending on user settings. Localized speed is rounded to compare only integer parts. **Android** Pass `isSpeedLimitExceeded` flag to `RoutingInfo`. Use this flag to mark speedometer red. **TODO**: Add unit-test to cover `routing::RoutingSession::IsSpeedLimitExceeded(...)` As far as I can see current GPS info with speed value is passed from Android to backend in the next way: 1. `LocationState.nativeLocationUpdated(time, lat, lon, accuracy, altitude, speed, bearing)` - wrappes values into `location::GpsInfo` instance and calls `android::Framework` 2. `android::Framework::OnLocationUpdated()` - passes GPS info to `::Framework` 3. `Framework::OnLocationUpdated()` - passes GPS into to `RoutingManager` 4. `RoutingManager::OnLocationUpdate()` - passes GPS info to `extrapolation::Extrapolator` 5. `extrapolation::Extrapolator::OnLocationUpdate()` - saves GPS info into field `m_lastGpsInfo`. And to get last known GPS location and speed I had to add methods: 1. `map::Framework::GetLastLocation()` which calls `RoutingManager` 2. `RoutingManager::GetLastLocation()` which calls `extrapolation::Extrapolator` 3. `extrapolation::Extrapolator::GetLastLocation()` which returns pointer to the field `m_lastGpsInfo`. Is this OK? ### Update 2024-09-05 Introduced new classes (name could be changed): ```cpp // C++ class SpeedFormatted { double m_speed; // Speed in km/h or mile/h depending on m_units. measurement_utils::Units m_units; // Metric or imperial. } ``` ```Java // Java class SpeedFormatted { public final double mSpeed; // Speed value in local units. public final String mSpeedStr; // Speed value formatted. public final SpeedFormatted.Units mUnits; // Two options "km/h" or "mi/h" } ``` So instead of using `Pair<String, String> StringUtils.nativeFormatSpeedAndUnits(double metersPerSecond)` method to get formatted speed we pass both current speed and speed limit as `SpeedFormatted` objects from Framework to NavMenu. And now `NavMenu` class decides if there is speeding by comparing speed values in "km/h" or "mi/h" units. **TODO:** Verify in the wild on a real road with speed limit. **TODO:** Add unit-test to cover `SpeedFormatted` class. **TODO:** check if iOS code need changes.

strump reviewed 2024-09-05 19:59:37 +00:00

android/app/src/main/java/app/organicmaps/Framework.java

					
				@ -195,3 +194,4 @@

				  public static native String nativeFormatSpeed(double speed); // TODO: move this method to StringUtils class.

				  public static native String nativeGetGe0Url(double lat, double lon, double zoomLevel, String name);

strump commented

2024-09-05 19:59:33 +00:00

I left these TODO comments to make changes later. Too many changes for a single PR.

I left these `TODO` comments to make changes later. Too many changes for a single PR.

android/app/src/main/java/app/organicmaps/widget/menu/NavMenu.java

					
				@ -224,2 +223,3 @@

				      mSpeedValue.setText(speed.mSpeedStr);

				    if (info.speedLimitMps > 0.0 && last.getSpeed() > info.speedLimitMps)

				    if (speedLimitExceeded)

strump commented

2024-09-05 19:57:35 +00:00

No more Pair<> classes. SpeedFormatted contains all details for UI.

No more `Pair<>` classes. `SpeedFormatted` contains all details for UI.

android/app/src/main/java/app/organicmaps/widget/menu/NavMenu.java

					
				@ -234,3 +234,3 @@

				    mSpeedUnits.setText(speedAndUnits.second);

				    mSpeedUnits.setText(info.speed.getUnitsStr(mActivity));

				    mSpeedViewContainer.setActivated(info.isSpeedCamLimitExceeded());

strump commented

2024-09-05 19:56:32 +00:00

Here we decide if there was speeding

biodranik (Migrated from github.com) reviewed 2024-09-06 11:27:36 +00:00

android/app/src/main/cpp/app/organicmaps/Framework.cpp

biodranik (Migrated from github.com) commented

2024-09-06 10:55:27 +00:00

nit: looks like a lot of some logic here. Should it be (later?) done only in the C++ core?

android/app/src/main/cpp/app/organicmaps/util/SpeedFormatted.hpp

					
				@ -0,0 +12,4 @@

				  jobject distanceObject = env->NewObject(

				      speedFormattedClass, speedFormattedConstructor,

				      speedFormatted.GetSpeed(), jni::ToJavaString(env, speedFormatted.GetSpeedString()), static_cast<uint8_t>(speedFormatted.GetUnits()));

biodranik (Migrated from github.com) commented

2024-09-06 11:17:56 +00:00

nit: Looks like a lot of code, with a lot of jni overhead. Can formatting/string preparation be done all in C++ code, passed as several strings (or maybe just one string returned as a concatenation of several strings?) to the iOS and Android UI just for displaying?

android/app/src/main/java/app/organicmaps/util/SpeedFormatted.java

					
				@ -0,0 +35,4 @@

				  public final String mSpeedStr;

				  public final SpeedFormatted.Units mUnits;

				  public SpeedFormatted(double mSpeed, @NonNull String mSpeedStr, byte unitsIndex)

biodranik (Migrated from github.com) commented

2024-09-06 11:19:40 +00:00

nit: Are there simpler ideas on the implementation? E.g. return an array of 3 strings instead of introducing a class? How will it work on iOS? On Qt/desktop platforms?

android/app/src/main/java/app/organicmaps/widget/menu/NavMenu.java

					
				@ -234,3 +234,3 @@

				    mSpeedUnits.setText(speedAndUnits.second);

				    mSpeedUnits.setText(info.speed.getUnitsStr(mActivity));

				    mSpeedViewContainer.setActivated(info.isSpeedCamLimitExceeded());

biodranik (Migrated from github.com) commented

2024-09-06 11:21:09 +00:00

Here we compare km with km, or mi with mi, right? What about the threshold idea?

platform/speed_formatted.cpp

					
				@ -0,0 +10,4 @@

				{

				using namespace measurement_utils;

				SpeedFormatted::SpeedFormatted(double speedMps) : SpeedFormatted(speedMps, GetMeasurementUnits()) {}

biodranik (Migrated from github.com) commented

2024-09-06 11:23:41 +00:00

It would be great to avoid introducing an implicit call to some external GetMeasurementUnits() method, and always pass it explicitly into the second constructor below.

platform/speed_formatted.cpp

					
				@ -0,0 +34,4 @@

				std::string SpeedFormatted::GetSpeedString() const

				{

				  if (!IsValid())

biodranik (Migrated from github.com) commented

2024-09-06 11:24:23 +00:00

What is the situation when this class can be invalid? Can we guarantee that when it is created, it is always valid?

platform/speed_formatted.cpp

					
				@ -0,0 +35,4 @@

				std::string SpeedFormatted::GetSpeedString() const

				{

				  if (!IsValid())

				    return "";

biodranik (Migrated from github.com) commented

2024-09-06 11:24:34 +00:00

nit: return {}

platform/speed_formatted.cpp

					
				@ -0,0 +47,4 @@

				  return ToStringPrecision(m_speed, precision);

				}

				std::string SpeedFormatted::GetUnitsString() const

biodranik (Migrated from github.com) commented

2024-09-06 11:26:17 +00:00

nit: We have in the UI:

Distance
Speed
Time
Elevation

There are already several classes for formatting. Does it make sense to write a single wrapper with simple interface for all these cases?

nit: We have in the UI: 1. Distance 2. Speed 3. Time 4. Elevation There are already several classes for formatting. Does it make sense to write a single wrapper with simple interface for all these cases?

platform/speed_formatted.hpp

					
				@ -0,0 +6,4 @@

				namespace platform

				{

				class SpeedFormatted

biodranik (Migrated from github.com) commented

2024-09-06 11:27:30 +00:00

nit: Can it be a struct with 3 values, filled by a single function call?

routing/routing_session.hpp

biodranik (Migrated from github.com) commented

2024-09-06 08:29:48 +00:00

Why not 0.0?

strump reviewed 2024-09-06 13:38:54 +00:00

android/app/src/main/java/app/organicmaps/util/SpeedFormatted.java

					
				@ -0,0 +35,4 @@

				  public final String mSpeedStr;

				  public final SpeedFormatted.Units mUnits;

				  public SpeedFormatted(double mSpeed, @NonNull String mSpeedStr, byte unitsIndex)

strump commented

2024-09-06 13:38:54 +00:00

In iOS code navigation UI get speed from iOS API and speed limit from DTO MWMNavigationDashboardEntity (see NavigationControlView.swift:159 ). iOS converts m/s to km/h or mi/h in UI code calling measurement_utils::MpsToUnits(...). Also it compares speed and limit values in m/s and doesn't do rounding.

To avoid JNI conversion from m/s to metric or imperial units I pass current speed and speed limit using SpeedFormatted class. So we have only one JNI call to Framework.nativeGetRouteFollowingInfo().

You propose to pass String speedStr, String speedLimitStr, byte speedUnits, bool isSpeedLimitExceeded instead, right?

In iOS code navigation UI get speed from iOS API and speed limit from DTO `MWMNavigationDashboardEntity` (see [NavigationControlView.swift:159](https://github.com/organicmaps/organicmaps/blob/master/iphone/Maps/Classes/CustomViews/NavigationDashboard/Views/NavigationControlView.swift#L159) ). iOS converts m/s to km/h or mi/h in UI code calling `measurement_utils::MpsToUnits(...)`. Also it compares speed and limit values in m/s and doesn't do rounding. To avoid JNI conversion from m/s to metric or imperial units I pass current speed and speed limit using `SpeedFormatted` class. So we have only one JNI call to `Framework.nativeGetRouteFollowingInfo()`. You propose to pass `String speedStr`, `String speedLimitStr`, `byte speedUnits`, `bool isSpeedLimitExceeded` instead, right?

strump reviewed 2024-09-06 13:42:34 +00:00

platform/speed_formatted.cpp

					
				@ -0,0 +34,4 @@

				std::string SpeedFormatted::GetSpeedString() const

				{

				  if (!IsValid())

strump commented

2024-09-06 13:42:34 +00:00

Speed limit could be 0.0 which means no limits at all. But if speed limit in OSM data is not set we have -1.0. So it makes sense to return "" if OSM data has no limit defined.

Speed limit could be `0.0` which means no limits at all. But if speed limit in OSM data is not set we have `-1.0`. So it makes sense to return `""` if OSM data has no limit defined.

biodranik (Migrated from github.com) reviewed 2024-09-06 18:53:26 +00:00

android/app/src/main/java/app/organicmaps/util/SpeedFormatted.java

					
				@ -0,0 +35,4 @@

				  public final String mSpeedStr;

				  public final SpeedFormatted.Units mUnits;

				  public SpeedFormatted(double mSpeed, @NonNull String mSpeedStr, byte unitsIndex)

biodranik (Migrated from github.com) commented

2024-09-06 18:53:26 +00:00

On iOS information comes from two methods:

- (void)updateFollowingInfo:(routing::FollowingInfo const &)info routePoints:(NSArray<MWMRoutePoint *> *)points type:(MWMRouterType)type

and

- (void)updateTransitInfo:(TransitRouteInfo const &)info

It would be great to have one source of truth on all platforms, without any logic. E.g. just strings to display and flags like "speed limit was hit", or better 3-state like "no speeding/limit hit/fine limit hit".

To avoid slow JNI calls, something like this may be used:
return currentSpeedStr.append(';').append(speedLimit).append(';').append(speedingFlag).apend(';').append(unitsStr);

Then it can be decoded in Java right before displaying.

Passing all values separately is also ok, just slower a bit.

On iOS information comes from two methods: ``` - (void)updateFollowingInfo:(routing::FollowingInfo const &)info routePoints:(NSArray<MWMRoutePoint *> *)points type:(MWMRouterType)type and - (void)updateTransitInfo:(TransitRouteInfo const &)info ``` It would be great to have one source of truth on all platforms, without any logic. E.g. just strings to display and flags like "speed limit was hit", or better 3-state like "no speeding/limit hit/fine limit hit". To avoid slow JNI calls, something like this may be used: `return currentSpeedStr.append(';').append(speedLimit).append(';').append(speedingFlag).apend(';').append(unitsStr);` Then it can be decoded in Java right before displaying. Passing all values separately is also ok, just slower a bit.

biodranik (Migrated from github.com) reviewed 2024-09-06 18:56:58 +00:00

platform/speed_formatted.cpp

					
				@ -0,0 +34,4 @@

				std::string SpeedFormatted::GetSpeedString() const

				{

				  if (!IsValid())

biodranik (Migrated from github.com) commented

2024-09-06 18:56:58 +00:00

Store "invalid" value in valid class doesn't look clean. C++ has std::optional for that, there are also nullptr and empty string that can be used in JNI context to define "missing" speed limit vs "unlimited". AFAIR unlimited roads are only in Germany, and passing there something like 999 should work fine too.

strump reviewed 2024-09-07 11:23:58 +00:00

android/app/src/main/java/app/organicmaps/util/SpeedFormatted.java

					
				@ -0,0 +35,4 @@

				  public final String mSpeedStr;

				  public final SpeedFormatted.Units mUnits;

				  public SpeedFormatted(double mSpeed, @NonNull String mSpeedStr, byte unitsIndex)

strump commented

2024-09-07 11:23:58 +00:00

As far as I can tell, iOS doesn't have problem with units and formatting. Swift calls C++ function from measurement_utils namespace to convert m/s to units-per-hour and format speed to string. No problems with Swift<->C++ calls, as far as I can see.

New approach when MWMRoutingManager for iOS and Framework.nativeGetRouteFollowingInfo() for Android do speed convertions and have some new protocol with ';' separators looks like overengineering to me.

I propose to drop SpeedFormatted code and let UI do units conversions and string formatting with plain Java. I think functions within measurement_utils namespace would be easy to port to Java. This would minimize objects allocation between C++ and Java (get back double speedLimitMps as in master). What does iOS devs think of this?

As far as I can tell, iOS doesn't have problem with units and formatting. Swift calls C++ function from `measurement_utils` namespace to convert m/s to units-per-hour and format speed to string. No problems with Swift<->C++ calls, as far as I can see. New approach when `MWMRoutingManager` for iOS and `Framework.nativeGetRouteFollowingInfo()` for Android do speed convertions and have some new protocol with `';'` separators looks like overengineering to me. I propose to drop `SpeedFormatted` code and let UI do units conversions and string formatting with plain Java. I think functions within `measurement_utils` namespace would be easy to port to Java. This would minimize objects allocation between C++ and Java (get back `double speedLimitMps` as in `master`). What does iOS devs think of this?

This repo is archived. You cannot comment on pull requests.

No reviewers