Cabana: cleanup code (#26369)

* remove ChartView::enterEvent

* cleanup ChartsWidget::removeAll

* group graphics items

* remove rubber->setPalette

* helper function DBCManager::parseId

* remove variable name from dbcManager

* fix readme/Usage

* use QSlider::setRange

* cleanup include

* use emplace_back

* remove varialbe routeName from CanMessages

* remove tmp variable

* remove blank line

* cleanup layout

* clean settings layout

* connect to streamStarted

* cleanup signal/slot

tools/cabana/README.md

@@ -8,7 +8,7 @@ Cabana is a tool developed to view raw CAN data. One use for this is creating an
 
 ```bash
 $ ./cabana -h
-Usage: ./_cabana [options] route
+Usage: ./cabana [options] route
 
 Options:
   -h, --help             Displays this help.

tools/cabana/canmessages.cc

@@ -1,6 +1,5 @@
 #include "tools/cabana/canmessages.h"
 
-#include <QDebug>
 #include <QSettings>
 
 #include "tools/cabana/dbcmanager.h"
@@ -9,7 +8,6 @@ CANMessages *can = nullptr;
 
 CANMessages::CANMessages(QObject *parent) : QObject(parent) {
   can = this;
-
   QObject::connect(this, &CANMessages::received, this, &CANMessages::process, Qt::QueuedConnection);
   QObject::connect(&settings, &Settings::changed, this, &CANMessages::settingChanged);
 }
@@ -24,11 +22,11 @@ static bool event_filter(const Event *e, void *opaque) {
 }
 
 bool CANMessages::loadRoute(const QString &route, const QString &data_dir, bool use_qcam) {
-  routeName = route;
   replay = new Replay(route, {"can", "roadEncodeIdx", "carParams"}, {}, nullptr, use_qcam ? REPLAY_FLAG_QCAMERA : 0, data_dir, this);
   replay->setSegmentCacheLimit(settings.cached_segment_limit);
   replay->installEventFilter(event_filter, this);
   QObject::connect(replay, &Replay::segmentsMerged, this, &CANMessages::eventsMerged);
+  QObject::connect(replay, &Replay::streamStarted, this, &CANMessages::streamStarted);
   if (replay->load()) {
     replay->start();
     return true;
@@ -40,12 +38,9 @@ QList<QPointF> CANMessages::findSignalValues(const QString &id, const Signal *si
   auto evts = events();
   if (!evts) return {};
 
-  auto l = id.split(':');
-  int bus = l[0].toInt();
-  uint32_t address = l[1].toUInt(nullptr, 16);
-
   QList<QPointF> ret;
   ret.reserve(max_count);
+  auto [bus, address] = DBCManager::parseId(id);
   for (auto &evt : *evts) {
     if (evt->which != cereal::Event::Which::CAN) continue;
 
@@ -101,10 +96,9 @@ bool CANMessages::eventFilter(const Event *event) {
       data.bus_time = c.getBusTime();
       data.dat.append((char *)c.getDat().begin(), c.getDat().size());
 
-      auto &count = counters[id];
+      data.count = ++counters[id];
-      data.count = ++count;
       if (double delta = (current_sec - counters_begin_sec); delta > 0) {
-        data.freq = count / delta;
+        data.freq = data.count / delta;
       }
       (*new_msgs)[id] = data;
     }

tools/cabana/canmessages.h

@@ -32,7 +32,7 @@ public:
   QList<QPointF> findSignalValues(const QString&id, const Signal* signal, double value, FindFlags flag, int max_count);
   bool eventFilter(const Event *event);
 
-  inline QString route() const { return routeName; }
+  inline QString route() const { return replay->route()->name(); }
   inline QString carFingerprint() const { return replay->carFingerprint().c_str(); }
   inline double totalSeconds() const { return replay->totalSeconds(); }
   inline double routeStartTime() const { return replay->routeStartTime() / (double)1e9; }
@@ -47,6 +47,7 @@ public:
   inline const std::vector<std::tuple<int, int, TimelineType>> getTimeline() { return replay->getTimeline(); }
 
 signals:
+  void streamStarted();
   void eventsMerged();
   void updated();
   void received(QHash<QString, CanData> *);
@@ -58,9 +59,7 @@ protected:
   void process(QHash<QString, CanData> *);
   void settingChanged();
 
-  QString routeName;
   Replay *replay = nullptr;
-
   std::mutex lock;
   std::atomic<double> counters_begin_sec = 0;
   QHash<QString, uint32_t> counters;

tools/cabana/chartswidget.cc

@@ -176,16 +176,8 @@ void ChartsWidget::removeChart(ChartWidget *chart) {
 }
 
 void ChartsWidget::removeAll(const Signal *sig) {
-  QMutableListIterator<ChartWidget *> it(charts);
+  for (auto c : charts.toVector())
-  while (it.hasNext()) {
+    if (!sig || c->signal == sig) removeChart(c);
-    auto c = it.next();
-    if (sig == nullptr || c->signal == sig) {
-      c->deleteLater();
-      emit chartClosed(c->id, c->signal);
-      it.remove();
-    }
-  }
-  updateTitleBar();
 }
 
 void ChartsWidget::signalUpdated(const Signal *sig) {
@@ -267,26 +259,21 @@ ChartView::ChartView(const QString &id, const Signal *sig, QWidget *parent)
   chart->setMargins({0, 0, 0, 0});
   chart->layout()->setContentsMargins(0, 0, 0, 0);
 
+  line_marker = new QGraphicsLineItem(chart);
+  line_marker->setZValue(chart->zValue() + 10);
+
   track_line = new QGraphicsLineItem(chart);
-  track_line->setZValue(chart->zValue() + 10);
   track_line->setPen(QPen(Qt::darkGray, 1, Qt::DashLine));
   track_ellipse = new QGraphicsEllipseItem(chart);
-  track_ellipse->setZValue(chart->zValue() + 10);
   track_ellipse->setBrush(Qt::darkGray);
   value_text = new QGraphicsTextItem(chart);
-  value_text->setZValue(chart->zValue() + 10);
+  item_group = scene()->createItemGroup({track_line, track_ellipse, value_text});
-  line_marker = new QGraphicsLineItem(chart);
+  item_group->setZValue(chart->zValue() + 10);
-  line_marker->setZValue(chart->zValue() + 10);
 
   setChart(chart);
 
   setRenderHint(QPainter::Antialiasing);
   setRubberBand(QChartView::HorizontalRubberBand);
-  if (auto rubber = findChild<QRubberBand *>()) {
-    QPalette pal;
-    pal.setBrush(QPalette::Base, QColor(0, 0, 0, 80));
-    rubber->setPalette(pal);
-  }
 
   QTimer *timer = new QTimer(this);
   timer->setInterval(100);
@@ -335,12 +322,9 @@ void ChartView::updateSeries(const std::pair<double, double> range) {
   auto events = can->events();
   if (!events) return;
 
-  auto l = id.split(':');
-  int bus = l[0].toInt();
-  uint32_t address = l[1].toUInt(nullptr, 16);
-
   vals.clear();
   vals.reserve((range.second - range.first) * 1000);  // [n]seconds * 1000hz
+  auto [bus, address] = DBCManager::parseId(id);
   double route_start_time = can->routeStartTime();
   Event begin_event(cereal::Event::Which::INIT_DATA, (route_start_time + range.first) * 1e9);
   auto begin = std::lower_bound(events->begin(), events->end(), &begin_event, Event::lessThan());
@@ -380,17 +364,8 @@ void ChartView::updateAxisY() {
   }
 }
 
-void ChartView::enterEvent(QEvent *event) {
-  track_line->setVisible(true);
-  value_text->setVisible(true);
-  track_ellipse->setVisible(true);
-  QChartView::enterEvent(event);
-}
-
 void ChartView::leaveEvent(QEvent *event) {
-  track_line->setVisible(false);
+  item_group->setVisible(false);
-  value_text->setVisible(false);
-  track_ellipse->setVisible(false);
   QChartView::leaveEvent(event);
 }
 
@@ -442,9 +417,7 @@ void ChartView::mouseMoveEvent(QMouseEvent *ev) {
       }
       value_text->setPos(text_x, pos.y() - 10);
     }
-    track_line->setVisible(value != vals.end());
+    item_group->setVisible(value != vals.end());
-    value_text->setVisible(value != vals.end());
-    track_ellipse->setVisible(value != vals.end());
   } else {
     setViewportUpdateMode(QGraphicsView::FullViewportUpdate);
   }

tools/cabana/chartswidget.h

@@ -8,7 +8,6 @@
 #include <QGraphicsTextItem>
 #include <QPushButton>
 #include <QVBoxLayout>
-#include <QWidget>
 #include <QtCharts/QChartView>
 
 #include "tools/cabana/canmessages.h"
@@ -33,11 +32,11 @@ signals:
 private:
   void mouseReleaseEvent(QMouseEvent *event) override;
   void mouseMoveEvent(QMouseEvent *ev) override;
-  void enterEvent(QEvent *event) override;
   void leaveEvent(QEvent *event) override;
   void adjustChartMargins();
   void updateAxisY();
 
+  QGraphicsItemGroup *item_group;
   QGraphicsLineItem *track_line;
   QGraphicsEllipseItem *track_ellipse;
   QGraphicsTextItem *value_text;

tools/cabana/dbcmanager.cc

@@ -9,8 +9,7 @@ DBCManager::DBCManager(QObject *parent) : QObject(parent) {}
 DBCManager::~DBCManager() {}
 
 void DBCManager::open(const QString &dbc_file_name) {
-  dbc_name = dbc_file_name;
+  dbc = const_cast<DBC *>(dbc_lookup(dbc_file_name.toStdString()));
-  dbc = const_cast<DBC *>(dbc_lookup(dbc_name.toStdString()));
   msg_map.clear();
   for (auto &msg : dbc->msgs) {
     msg_map[msg.address] = &msg;
@@ -19,7 +18,6 @@ void DBCManager::open(const QString &dbc_file_name) {
 }
 
 void DBCManager::open(const QString &name, const QString &content) {
-  this->dbc_name = name;
   std::istringstream stream(content.toStdString());
   dbc = const_cast<DBC *>(dbc_parse_from_stream(name.toStdString(), stream));
   msg_map.clear();
@@ -51,22 +49,19 @@ QString DBCManager::generateDBC() {
 }
 
 void DBCManager::updateMsg(const QString &id, const QString &name, uint32_t size) {
-  auto m = const_cast<Msg *>(msg(id));
+  if (auto m = const_cast<Msg *>(msg(id))) {
-  if (m) {
     m->name = name.toStdString();
     m->size = size;
   } else {
-    uint32_t address = addressFromId(id);
+    m = &dbc->msgs.emplace_back(Msg{.address = parseId(id).second, .name = name.toStdString(), .size = size});
-    dbc->msgs.push_back({.address = address, .name = name.toStdString(), .size = size});
+    msg_map[m->address] = m;
-    msg_map[address] = &dbc->msgs.back();
   }
   emit msgUpdated(id);
 }
 
 void DBCManager::addSignal(const QString &id, const Signal &sig) {
   if (Msg *m = const_cast<Msg *>(msg(id))) {
-    m->sigs.push_back(sig);
+    emit signalAdded(&m->sigs.emplace_back(sig));
-    emit signalAdded(&m->sigs.back());
   }
 }
 
@@ -90,8 +85,9 @@ void DBCManager::removeSignal(const QString &id, const QString &sig_name) {
   }
 }
 
-uint32_t DBCManager::addressFromId(const QString &id) {
+std::pair<uint8_t, uint32_t> DBCManager::parseId(const QString &id) {
-  return id.mid(id.indexOf(':') + 1).toUInt(nullptr, 16);
+  const auto list = id.split(':');
+  return {list[0].toInt(), list[1].toUInt(nullptr, 16)};
 }
 
 DBCManager *dbc() {

tools/cabana/dbcmanager.h

@@ -18,13 +18,13 @@ public:
   void updateSignal(const QString &id, const QString &sig_name, const Signal &sig);
   void removeSignal(const QString &id, const QString &sig_name);
 
-  static uint32_t addressFromId(const QString &id);
+  static std::pair<uint8_t, uint32_t> parseId(const QString &id);
   inline static std::vector<std::string> allDBCNames() { return get_dbc_names(); }
-  inline QString name() const { return dbc_name; }
+  inline QString name() const { return dbc ? dbc->name.c_str() : ""; }
 
   void updateMsg(const QString &id, const QString &name, uint32_t size);
   inline const DBC *getDBC() const { return dbc; }
-  inline const Msg *msg(const QString &id) const { return msg(addressFromId(id)); }
+  inline const Msg *msg(const QString &id) const { return msg(parseId(id).second); }
   inline const Msg *msg(uint32_t address) const {
     auto it = msg_map.find(address);
     return it != msg_map.end() ? it->second : nullptr;
@@ -38,7 +38,6 @@ signals:
   void DBCFileChanged();
 
 private:
-  QString dbc_name;
   DBC *dbc = nullptr;
   std::unordered_map<uint32_t, const Msg *> msg_map;
 };

tools/cabana/detailwidget.cc

@@ -277,9 +277,7 @@ void DetailWidget::removeSignal(const Signal *sig) {
 
 EditMessageDialog::EditMessageDialog(const QString &msg_id, const QString &title, int size, QWidget *parent) : QDialog(parent) {
   setWindowTitle(tr("Edit message"));
-  QVBoxLayout *main_layout = new QVBoxLayout(this);
+  QFormLayout *form_layout = new QFormLayout(this);
-
-  QFormLayout *form_layout = new QFormLayout();
   form_layout->addRow("ID", new QLabel(msg_id));
 
   name_edit = new QLineEdit(title, this);
@@ -291,10 +289,8 @@ EditMessageDialog::EditMessageDialog(const QString &msg_id, const QString &title
   size_spin->setValue(size);
   form_layout->addRow(tr("Size"), size_spin);
 
-  main_layout->addLayout(form_layout);
-
   auto buttonBox = new QDialogButtonBox(QDialogButtonBox::Ok | QDialogButtonBox::Cancel);
-  main_layout->addWidget(buttonBox);
+  form_layout->addRow(buttonBox);
   setFixedWidth(parent->width() * 0.9);
 
   connect(buttonBox, &QDialogButtonBox::accepted, this, &QDialog::accept);

tools/cabana/detailwidget.h

@@ -26,9 +26,6 @@ public:
   void setMessage(const QString &message_id);
   void dbcMsgChanged(int show_form_idx = -1);
 
-signals:
-  void binaryViewMoved(bool in);
-
 private:
   void updateChartState(const QString &id, const Signal *sig, bool opened);
   void showTabBarContextMenu(const QPoint &pt);

tools/cabana/mainwin.cc

@@ -81,11 +81,10 @@ MainWindow::MainWindow() : QWidget() {
   QObject::connect(this, &MainWindow::showMessage, status_bar, &QStatusBar::showMessage);
   QObject::connect(this, &MainWindow::updateProgressBar, this, &MainWindow::updateDownloadProgress);
   QObject::connect(messages_widget, &MessagesWidget::msgSelectionChanged, detail_widget, &DetailWidget::setMessage);
-  QObject::connect(detail_widget, &DetailWidget::binaryViewMoved, [this](bool in) { splitter->setSizes({in ? 100 : 0, 500}); });
   QObject::connect(charts_widget, &ChartsWidget::dock, this, &MainWindow::dockCharts);
   QObject::connect(charts_widget, &ChartsWidget::rangeChanged, video_widget, &VideoWidget::rangeChanged);
   QObject::connect(settings_btn, &QPushButton::clicked, this, &MainWindow::setOption);
-  QObject::connect(can, &CANMessages::eventsMerged, [=]() { fingerprint_label->setText(can->carFingerprint() ); });
+  QObject::connect(can, &CANMessages::streamStarted, [=]() { fingerprint_label->setText(can->carFingerprint() ); });
 
   main_win = this;
   qInstallMessageHandler(qLogMessageHandler);

tools/cabana/messageswidget.cc

@@ -67,7 +67,7 @@ MessagesWidget::MessagesWidget(QWidget *parent) : QWidget(parent) {
 
   // signals/slots
   QObject::connect(filter, &QLineEdit::textChanged, model, &MessageListModel::setFilterString);
-  QObject::connect(can, &CANMessages::eventsMerged, this, &MessagesWidget::loadDBCFromFingerprint);
+  QObject::connect(can, &CANMessages::streamStarted, this, &MessagesWidget::loadDBCFromFingerprint);
   QObject::connect(can, &CANMessages::updated, [this]() { model->updateState(); });
   QObject::connect(dbc_combo, SIGNAL(activated(const QString &)), SLOT(loadDBCFromName(const QString &)));
   QObject::connect(load_from_paste, &QPushButton::clicked, this, &MessagesWidget::loadDBCFromPaste);

tools/cabana/settings.cc

@@ -36,8 +36,7 @@ void Settings::load() {
 
 SettingsDlg::SettingsDlg(QWidget *parent) : QDialog(parent) {
   setWindowTitle(tr("Settings"));
-  QVBoxLayout *main_layout = new QVBoxLayout(this);
+  QFormLayout *form_layout = new QFormLayout(this);
-  QFormLayout *form_layout = new QFormLayout();
 
   fps = new QSpinBox(this);
   fps->setRange(10, 100);
@@ -74,10 +73,8 @@ SettingsDlg::SettingsDlg(QWidget *parent) : QDialog(parent) {
   chart_theme->setCurrentIndex(settings.chart_theme == 1 ? 1 : 0);
   form_layout->addRow(tr("Chart theme"), chart_theme);
 
-  main_layout->addLayout(form_layout);
-
   auto buttonBox = new QDialogButtonBox(QDialogButtonBox::Ok | QDialogButtonBox::Cancel);
-  main_layout->addWidget(buttonBox);
+  form_layout->addRow(buttonBox);
 
   setFixedWidth(360);
   connect(buttonBox, &QDialogButtonBox::accepted, this, &SettingsDlg::save);

tools/cabana/videowidget.cc

@@ -28,11 +28,9 @@ VideoWidget::VideoWidget(QWidget *parent) : QWidget(parent) {
 
   slider = new Slider(this);
   slider->setSingleStep(0);
-  slider->setMinimum(0);
-  slider->setMaximum(can->totalSeconds() * 1000);
   slider_layout->addWidget(slider);
 
-  end_time_label = new QLabel(formatTime(can->totalSeconds()));
+  end_time_label = new QLabel(this);
   slider_layout->addWidget(end_time_label);
   main_layout->addLayout(slider_layout);
 
@@ -61,6 +59,10 @@ VideoWidget::VideoWidget(QWidget *parent) : QWidget(parent) {
   QObject::connect(slider, &QSlider::valueChanged, [=](int value) { time_label->setText(formatTime(value / 1000)); });
   QObject::connect(cam_widget, &CameraWidget::clicked, [this]() { pause(!can->isPaused()); });
   QObject::connect(play_btn, &QPushButton::clicked, [=]() { pause(!can->isPaused()); });
+  QObject::connect(can, &CANMessages::streamStarted, [this]() {
+    end_time_label->setText(formatTime(can->totalSeconds()));
+    slider->setRange(0, can->totalSeconds() * 1000);
+  });
 }
 
 void VideoWidget::pause(bool pause) {
@@ -74,8 +76,7 @@ void VideoWidget::rangeChanged(double min, double max, bool is_zoomed) {
     max = can->totalSeconds();
   }
   end_time_label->setText(formatTime(max));
-  slider->setMinimum(min * 1000);
+  slider->setRange(min * 1000, max * 1000);
-  slider->setMaximum(max * 1000);
 }
 
 void VideoWidget::updateState() {
@@ -91,7 +92,7 @@ Slider::Slider(QWidget *parent) : QSlider(Qt::Horizontal, parent) {
     timeline = can->getTimeline();
     update();
   });
-  timer->start();
+  QObject::connect(can, SIGNAL(streamStarted()), timer, SLOT(start()));
 }
 
 void Slider::sliderChange(QAbstractSlider::SliderChange change) {

tools/cabana/videowidget.h

@@ -3,7 +3,6 @@
 #include <QLabel>
 #include <QPushButton>
 #include <QSlider>
-#include <QWidget>
 
 #include "selfdrive/ui/qt/widgets/cameraview.h"
 #include "tools/cabana/canmessages.h"