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

ui: generate JWT on every (re-)route #32480

Closed
wants to merge 8 commits into from
Closed

Conversation

BBBmau
Copy link
Contributor

@BBBmau BBBmau commented May 19, 2024

After a better understanding of the problem, it made sense to match when JWT is generated in navd which is where the route is calculated.

For ui, this would be when a new route is received.

i left the other places where JWT is generated as is (initial boot and onroadTransition) and just added an update to the m_settings within where the new route is received. I also updated the expiry time to match navd.

Resolves #32398

Copy link
Contributor

github-actions bot commented May 19, 2024

Thanks for contributing to openpilot! In order for us to review your PR as quickly as possible, check the following:

  • Convert your PR to a draft unless it's ready to review
  • Read the contributing docs
  • Before marking as "ready for review", ensure:
    • the goal is clearly stated in the description
    • all the tests are passing
    • the change is something we merge
    • include a route or your device' dongle ID if relevant

@BBBmau
Copy link
Contributor Author

BBBmau commented May 20, 2024

since the solution is to generate JWT for every request, I've added the JWT generation when we Update current location marker and Update Destination Location Marker

@BBBmau BBBmau changed the title ui: generate JWT on every new route ui: generate JWT on every new route and every update on current/destination marker May 20, 2024
selfdrive/ui/qt/maps/map_helpers.cc Outdated Show resolved Hide resolved
@@ -362,6 +364,7 @@ void MapWindow::offroadTransition(bool offroad) {
}

void MapWindow::updateDestinationMarker() {
m_settings.setApiKey(get_mapbox_token());
Copy link
Contributor

Choose a reason for hiding this comment

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

you sure this updates the settings for the underlying MapLibre widget?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, The MapWindow class has both m_settings and m_map as private and since we're updating the setting within the class it would be updating the setting of the MapLibre widget

class MapWindow : public QOpenGLWidget {
  Q_OBJECT

public:
  MapWindow(const QMapLibre::Settings &);
  ~MapWindow();

private:
  void initializeGL() final;
  void paintGL() final;
  void resizeGL(int w, int h) override;

  QMapLibre::Settings m_settings;
  QScopedPointer<QMapLibre::Map> m_map;

An example of this can be seen with m_map where we make updates to the QMapLibre::map itself such as setting the zoom and coordinates:

  if (interaction_counter == 0) {
    if (last_position) m_map->setCoordinate(*last_position);
    if (last_bearing) m_map->setBearing(*last_bearing);
    m_map->setZoom(util::map_val<float>(velocity_filter.x(), 0, 30, MAX_ZOOM, MIN_ZOOM));
  } else {
    interaction_counter--;
  }

@BBBmau BBBmau requested a review from adeebshihadeh May 21, 2024 01:53
@github-actions github-actions bot added the ui label May 24, 2024
@BBBmau
Copy link
Contributor Author

BBBmau commented May 24, 2024

I finally was able to test this personally (car had been in repair for little over a week) and didn't see any issues.

Route w/ GPS: 9a31d18cd1a79c69/00000001--2a2cef9ef7/0

anything else needed? @adeebshihadeh

@adeebshihadeh
Copy link
Contributor

Same as every PR - we just need to find some time to validate it.

Copy link
Contributor

@sshane sshane left a comment

Choose a reason for hiding this comment

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

let's only update the token when getting a new navRoute from navd (when it gets its own new token, as you found)

selfdrive/ui/qt/maps/map.cc Outdated Show resolved Hide resolved
selfdrive/ui/qt/maps/map.cc Outdated Show resolved Hide resolved
selfdrive/ui/qt/maps/map.cc Outdated Show resolved Hide resolved
@sshane sshane changed the title ui: generate JWT on every new route and every update on current/destination marker ui: generate JWT on every (re-)route May 29, 2024
selfdrive/ui/qt/maps/map.cc Outdated Show resolved Hide resolved
@sshane
Copy link
Contributor

sshane commented May 29, 2024

This also doesn't work because the internal map object only gets the token from the settings on init: https://github.com/maplibre/maplibre-native-qt/blob/3a83b8ecfd961983cf3670a145ef6176333dd443/src/core/map.cpp#L1617

This is seen when testing as well, you can open the map with an invalid date, then fix it and send a new nav route; it never loads.

@sshane
Copy link
Contributor

sshane commented May 30, 2024

Closing in favor of #32571

@sshane sshane closed this May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ui: map doesn't load if time is wrong at the start
3 participants