Cabana: auto update signal on field changes (#26464)

* auto update signal on field changes

* better icon
old-commit-hash: 3c507e8ad7

tools/cabana/dbcmanager.cc

@@ -165,3 +165,11 @@ std::pair<int, int> getSignalRange(const Signal *s) {
   int to = from + s->size - 1;
   return {from, to};
 }
+
+bool operator==(const Signal &l, const Signal &r) {
+  return l.name == r.name && l.size == r.size &&
+         l.start_bit == r.start_bit &&
+         l.msb == r.msb && l.lsb == r.lsb &&
+         l.is_signed == r.is_signed && l.is_little_endian == r.is_little_endian &&
+         l.factor == r.factor && l.offset == r.offset;
+}

tools/cabana/dbcmanager.h

@@ -54,6 +54,8 @@ private:
 
 // TODO: Add helper function in dbc.h
 double get_raw_value(uint8_t *data, size_t data_size, const Signal &sig);
+bool operator==(const Signal &l, const Signal &r);
+inline bool operator!=(const Signal &l, const Signal &r) { return !(l == r); }
 int bigEndianStartBitsIndex(int start_bit);
 int bigEndianBitIndex(int index);
 void updateSigSizeParamsFromRange(Signal &s, int start_bit, int size);

tools/cabana/detailwidget.cc

@@ -6,7 +6,6 @@
 #include <QMessageBox>
 #include <QScrollBar>
 #include <QTimer>
-#include <QVBoxLayout>
 
 #include "selfdrive/ui/qt/util.h"
 #include "tools/cabana/canmessages.h"
@@ -81,9 +80,8 @@ DetailWidget::DetailWidget(ChartsWidget *charts, QWidget *parent) : charts(chart
   container_layout->addWidget(binary_view);
 
   // signals
-  signals_container = new QWidget(this);
+  signals_layout = new QVBoxLayout();
-  signals_container->setLayout(new QVBoxLayout);
+  container_layout->addLayout(signals_layout);
-  container_layout->addWidget(signals_container);
 
   // history log
   history_log = new HistoryLog(this);
@@ -114,36 +112,25 @@ void DetailWidget::showTabBarContextMenu(const QPoint &pt) {
     QMenu menu(this);
     menu.addAction(tr("Close Other Tabs"));
     if (menu.exec(tabbar->mapToGlobal(pt))) {
-      tabbar->setCurrentIndex(index);
+      tabbar->moveTab(index, 0);
-      // remove all tabs before the one to keep
+      tabbar->setCurrentIndex(0);
-      for (int i = 0; i < index; ++i) {
+      while (tabbar->count() > 1)
-        tabbar->removeTab(0);
-      }
-      // remove all tabs after the one to keep
-      while (tabbar->count() > 1) {
         tabbar->removeTab(1);
-      }
     }
   }
 }
 
 void DetailWidget::setMessage(const QString &message_id) {
-  if (message_id.isEmpty()) return;
-
-  int index = -1;
-  for (int i = 0; i < tabbar->count(); ++i) {
-    if (tabbar->tabText(i) == message_id) {
-      index = i;
-      break;
-    }
-  }
   msg_id = message_id;
+  int index = tabbar->count() - 1;
+  for (/**/; index >= 0 && tabbar->tabText(index) != msg_id; --index) { /**/ }
   if (index == -1) {
     index = tabbar->addTab(message_id);
     tabbar->setTabToolTip(index, msgName(message_id));
   }
   tabbar->setCurrentIndex(index);
   dbcMsgChanged();
+  scroll->verticalScrollBar()->setValue(0);
 }
 
 void DetailWidget::dbcMsgChanged(int show_form_idx) {
@@ -154,48 +141,42 @@ void DetailWidget::dbcMsgChanged(int show_form_idx) {
   binary_view->setMessage(msg_id);
   history_log->setMessage(msg_id);
 
+  int i = 0;
   QStringList warnings;
-  for (auto f : signal_list) f->hide();
-
   const DBCMsg *msg = dbc()->msg(msg_id);
   if (msg) {
-    int i = 0;
     for (auto &[name, sig] : msg->sigs) {
       SignalEdit *form = i < signal_list.size() ? signal_list[i] : nullptr;
       if (!form) {
         form = new SignalEdit(i);
-        QObject::connect(form, &SignalEdit::showFormClicked, this, &DetailWidget::showForm);
         QObject::connect(form, &SignalEdit::remove, this, &DetailWidget::removeSignal);
         QObject::connect(form, &SignalEdit::save, this, &DetailWidget::saveSignal);
         QObject::connect(form, &SignalEdit::highlight, binary_view, &BinaryView::highlight);
         QObject::connect(binary_view, &BinaryView::signalHovered, form, &SignalEdit::signalHovered);
         QObject::connect(form, &SignalEdit::showChart, charts, &ChartsWidget::showChart);
-        signals_container->layout()->addWidget(form);
+        signals_layout->addWidget(form);
         signal_list.push_back(form);
       }
-      form->setSignal(msg_id, &sig, i == show_form_idx);
+      form->setSignal(msg_id, &sig);
       form->setChartOpened(charts->isChartOpened(msg_id, &sig));
-      form->show();
       ++i;
     }
     if (msg->size != can->lastMessage(msg_id).dat.size())
       warnings.push_back(tr("Message size (%1) is incorrect.").arg(msg->size));
   }
+  for (/**/; i < signal_list.size(); ++i)
+    signal_list[i]->hide();
 
   toolbar->setVisible(!msg_id.isEmpty());
   remove_msg_act->setEnabled(msg != nullptr);
   name_label->setText(msgName(msg_id));
 
-  // Check overlapping bits
+  for (auto s : binary_view->getOverlappingSignals())
-  if (auto overlapping = binary_view->getOverlappingSignals(); !overlapping.isEmpty()) {
+    warnings.push_back(tr("%1 has overlapping bits.").arg(s->name.c_str()));
-    for (auto s : overlapping)
-      warnings.push_back(tr("%1 has overlapping bits.").arg(s->name.c_str()));
-  }
 
   warning_label->setText(warnings.join('\n'));
   warning_widget->setVisible(!warnings.isEmpty());
-  setUpdatesEnabled(true);
+  QTimer::singleShot(1, [this]() { setUpdatesEnabled(true); });
-  scroll->verticalScrollBar()->setValue(0);
 }
 
 void DetailWidget::updateState() {
@@ -206,14 +187,6 @@ void DetailWidget::updateState() {
   history_log->updateState();
 }
 
-void DetailWidget::showForm() {
-  SignalEdit *sender = qobject_cast<SignalEdit *>(QObject::sender());
-  setUpdatesEnabled(false);
-  for (auto f : signal_list)
-    f->setFormVisible(f == sender && !f->isFormVisible());
-  QTimer::singleShot(1, [this]() { setUpdatesEnabled(true); });
-}
-
 void DetailWidget::updateChartState(const QString &id, const Signal *sig, bool opened) {
   for (auto f : signal_list)
     if (f->msg_id == id && f->sig == sig) f->setChartOpened(opened);
@@ -230,9 +203,7 @@ void DetailWidget::editMsg() {
 }
 
 void DetailWidget::removeMsg() {
-  if (auto msg = dbc()->msg(msg_id)) {
+  undo_stack->push(new RemoveMsgCommand(msg_id));
-    undo_stack->push(new RemoveMsgCommand(msg_id));
-  }
 }
 
 void DetailWidget::addSignal(int start_bit, int size, bool little_endian) {
@@ -248,13 +219,11 @@ void DetailWidget::addSignal(int start_bit, int size, bool little_endian) {
       }
     }
   }
-  Signal sig = {};
+  Signal sig = {.is_little_endian = little_endian};
   for (int i = 1; /**/; ++i) {
     sig.name = "NEW_SIGNAL_" + std::to_string(i);
-    auto it = msg->sigs.find(sig.name.c_str());
+    if (msg->sigs.count(sig.name.c_str()) == 0) break;
-    if (it == msg->sigs.end()) break;
   }
-  sig.is_little_endian = little_endian;
   updateSigSizeParamsFromRange(sig, start_bit, size);
   undo_stack->push(new AddSigCommand(msg_id, sig));
 }

tools/cabana/detailwidget.h

@@ -11,8 +11,6 @@
 #include "tools/cabana/signaledit.h"
 
 class EditMessageDialog : public QDialog {
-  Q_OBJECT
-
 public:
   EditMessageDialog(const QString &msg_id, const QString &title, int size, QWidget *parent);
 
@@ -38,13 +36,12 @@ private:
   void removeSignal(const Signal *sig);
   void editMsg();
   void removeMsg();
-  void showForm();
   void updateState();
 
   QString msg_id;
   QLabel *name_label, *time_label, *warning_label;
   QWidget *warning_widget;
-  QWidget *signals_container;
+  QVBoxLayout *signals_layout;
   QTabBar *tabbar;
   QToolBar *toolbar;
   QAction *remove_msg_act;

tools/cabana/signaledit.cc

@@ -1,12 +1,11 @@
 #include "tools/cabana/signaledit.h"
 
-#include <QDialogButtonBox>
 #include <QDoubleValidator>
 #include <QFormLayout>
 #include <QHBoxLayout>
-#include <QMessageBox>
-#include <QRadioButton>
 #include <QScrollArea>
+#include <QTimer>
+#include <QToolBar>
 #include <QVBoxLayout>
 
 #include "selfdrive/ui/qt/util.h"
@@ -15,7 +14,6 @@
 
 SignalForm::SignalForm(QWidget *parent) : QWidget(parent) {
   QFormLayout *form_layout = new QFormLayout(this);
-  form_layout->setContentsMargins(0, 0, 0, 0);
 
   name = new QLineEdit();
   form_layout->addRow(tr("Name"), name);
@@ -58,6 +56,13 @@ SignalForm::SignalForm(QWidget *parent) : QWidget(parent) {
   form_layout->addRow(tr("Maximum value"), max_val);
   val_desc = new QLineEdit();
   form_layout->addRow(tr("Value descriptions"), val_desc);
+
+  QObject::connect(name, &QLineEdit::textEdited, this, &SignalForm::changed);
+  QObject::connect(factor, &QLineEdit::textEdited, this, &SignalForm::changed);
+  QObject::connect(offset, &QLineEdit::textEdited, this, &SignalForm::changed);
+  QObject::connect(sign, SIGNAL(activated(int)), SIGNAL(changed()));
+  QObject::connect(endianness, SIGNAL(activated(int)), SIGNAL(changed()));
+  QObject::connect(size, SIGNAL(valueChanged(int)), SIGNAL(changed()));
 }
 
 // SignalEdit
@@ -67,36 +72,25 @@ SignalEdit::SignalEdit(int index, QWidget *parent) : form_idx(index), QWidget(pa
   main_layout->setContentsMargins(0, 0, 0, 0);
 
   // title bar
-  QHBoxLayout *title_layout = new QHBoxLayout();
+  auto toolbar = new QToolBar(this);
+  toolbar->setStyleSheet("QToolButton {width:15px;height:15px;font-size:15px}");
   icon = new QLabel();
-  title_layout->addWidget(icon);
+  toolbar->addWidget(icon);
   title = new ElidedLabel(this);
+  title->setSizePolicy(QSizePolicy::Expanding, QSizePolicy::Preferred);
   title->setStyleSheet(QString("font-weight:bold; color:%1").arg(getColor(index)));
-  title_layout->addWidget(title, 1);
+  toolbar->addWidget(title);
-
+  plot_btn = toolbar->addAction("", [this]() { emit showChart(msg_id, sig, !chart_opened); });
-  QPushButton *seek_btn = new QPushButton("⌕");
+  auto seek_btn = toolbar->addAction(QIcon::fromTheme("edit-find"), "", [this]() { SignalFindDlg(msg_id, sig, this).exec(); });
-  seek_btn->setStyleSheet("QPushButton{font-weight:bold;font-size:18px}");
   seek_btn->setToolTip(tr("Find signal values"));
-  seek_btn->setFixedSize(25, 25);
+  auto remove_btn = toolbar->addAction("x", [this]() { emit remove(sig); });
-  title_layout->addWidget(seek_btn);
+  remove_btn->setToolTip(tr("Remove signal"));
-
+  main_layout->addWidget(toolbar);
-  plot_btn = new QPushButton(this);
-  plot_btn->setStyleSheet("QPushButton {font-size:18px}");
-  plot_btn->setFixedSize(25, 25);
-  title_layout->addWidget(plot_btn);
-  main_layout->addLayout(title_layout);
 
   // signal form
-  form_container = new QWidget(this);
+  form = new SignalForm(this);
-  QVBoxLayout *v_layout = new QVBoxLayout(form_container);
+  form->setVisible(false);
-  QHBoxLayout *h = new QHBoxLayout();
+  main_layout->addWidget(form);
-  QPushButton *remove_btn = new QPushButton(tr("Remove Signal"));
-  h->addWidget(remove_btn);
-  h->addStretch();
-  QPushButton *save_btn = new QPushButton(tr("Save"));
-  h->addWidget(save_btn);
-  v_layout->addLayout(h);
-  main_layout->addWidget(form_container);
 
   // bottom line
   QFrame *hline = new QFrame();
@@ -104,25 +98,22 @@ SignalEdit::SignalEdit(int index, QWidget *parent) : form_idx(index), QWidget(pa
   hline->setFrameShadow(QFrame::Sunken);
   main_layout->addWidget(hline);
 
-  QObject::connect(remove_btn, &QPushButton::clicked, [this]() { emit remove(this->sig); });
+  QObject::connect(form, &SignalForm::changed, this, &SignalEdit::saveSignal);
   QObject::connect(title, &ElidedLabel::clicked, this, &SignalEdit::showFormClicked);
-  QObject::connect(save_btn, &QPushButton::clicked, this, &SignalEdit::saveSignal);
-  QObject::connect(plot_btn, &QPushButton::clicked, [this]() { emit showChart(msg_id, sig, !chart_opened); });
-  QObject::connect(seek_btn, &QPushButton::clicked, [this]() {
-    SignalFindDlg dlg(msg_id, sig, this);
-    dlg.exec();
-  });
   setSizePolicy(QSizePolicy::Preferred, QSizePolicy::Fixed);
 }
 
-void SignalEdit::setSignal(const QString &message_id, const Signal *signal, bool show_form) {
+void SignalEdit::setSignal(const QString &message_id, const Signal *signal) {
-  msg_id = message_id;
   sig = signal;
+  updateForm(msg_id == message_id && form->isVisible());
+  msg_id = message_id;
   title->setText(QString("%1. %2").arg(form_idx + 1).arg(sig->name.c_str()));
-  setFormVisible(show_form);
+  show();
 }
 
 void SignalEdit::saveSignal() {
+  if (!sig || !form->changed_by_user) return;
+
   Signal s = *sig;
   s.name = form->name->text().toStdString();
   s.size = form->size->text().toInt();
@@ -148,33 +139,38 @@ void SignalEdit::saveSignal() {
     s.lsb = bigEndianStartBitsIndex(bigEndianBitIndex(s.start_bit) + s.size - 1);
     s.msb = s.start_bit;
   }
-  title->setText(QString("%1. %2").arg(form_idx + 1).arg(form->name->text()));
+  if (s != *sig)
-  emit save(this->sig, s);
+    emit save(this->sig, s);
 }
 
 void SignalEdit::setChartOpened(bool opened) {
   plot_btn->setText(opened ? "☒" : "📈");
-  plot_btn->setToolTip(opened ? tr("Close Plot") :tr("Show Plot"));
+  plot_btn->setToolTip(opened ? tr("Close Plot") : tr("Show Plot"));
   chart_opened = opened;
 }
 
-void SignalEdit::setFormVisible(bool visible) {
+void SignalEdit::updateForm(bool visible) {
-  if (visible) {
+  if (visible && sig) {
-    if (!form) {
+    form->changed_by_user = false;
-      form = new SignalForm(this);
-      ((QVBoxLayout *)form_container->layout())->insertWidget(0, form);
-    }
     form->name->setText(sig->name.c_str());
-    form->size->setValue(sig->size);
     form->endianness->setCurrentIndex(sig->is_little_endian ? 0 : 1);
     form->sign->setCurrentIndex(sig->is_signed ? 0 : 1);
     form->factor->setText(QString::number(sig->factor));
     form->offset->setText(QString::number(sig->offset));
     form->msb->setText(QString::number(sig->msb));
     form->lsb->setText(QString::number(sig->lsb));
+    form->size->setValue(sig->size);
+    form->changed_by_user = true;
   }
-  form_container->setVisible(visible);
+  form->setVisible(visible);
-  icon->setText(visible ? "▼" : ">");
+  icon->setText(visible ? "▼ " : "> ");
+}
+
+void SignalEdit::showFormClicked() {
+  parentWidget()->setUpdatesEnabled(false);
+  for (auto &edit : parentWidget()->findChildren<SignalEdit*>())
+    edit->updateForm(edit == this && !form->isVisible());
+  QTimer::singleShot(1, [this]() { parentWidget()->setUpdatesEnabled(true); });
 }
 
 void SignalEdit::signalHovered(const Signal *s) {
@@ -182,6 +178,13 @@ void SignalEdit::signalHovered(const Signal *s) {
   title->setStyleSheet(QString("font-weight:bold; color:%1").arg(color.name()));
 }
 
+void SignalEdit::hideEvent(QHideEvent *event) {
+  msg_id = "";
+  sig = nullptr;
+  updateForm(false);
+  QWidget::hideEvent(event);
+}
+
 void SignalEdit::enterEvent(QEvent *event) {
   emit highlight(sig);
   QWidget::enterEvent(event);
@@ -204,7 +207,7 @@ SignalFindDlg::SignalFindDlg(const QString &id, const Signal *signal, QWidget *p
   comp_box->addItems({">", "=", "<"});
   h->addWidget(comp_box);
   QLineEdit *value_edit = new QLineEdit("0", this);
-  value_edit->setValidator( new QDoubleValidator(-500000, 500000, 6, this) );
+  value_edit->setValidator(new QDoubleValidator(-500000, 500000, 6, this));
   h->addWidget(value_edit, 1);
   QPushButton *search_btn = new QPushButton(tr("Find"), this);
   h->addWidget(search_btn);

tools/cabana/signaledit.h

@@ -1,11 +1,12 @@
 #pragma once
 
+#include <QAction>
 #include <QComboBox>
 #include <QDialog>
 #include <QLabel>
 #include <QLineEdit>
-#include <QPushButton>
 #include <QSpinBox>
+#include <QToolButton>
 
 #include "selfdrive/ui/qt/widgets/controls.h"
 
@@ -13,13 +14,17 @@
 #include "tools/cabana/dbcmanager.h"
 
 class SignalForm : public QWidget {
+  Q_OBJECT
 public:
   SignalForm(QWidget *parent);
-
   QLineEdit *name, *unit, *comment, *val_desc, *offset, *factor, *min_val, *max_val;
   QLabel *lsb, *msb;
   QSpinBox *size;
   QComboBox *sign, *endianness;
+  bool changed_by_user = false;
+
+ signals:
+  void changed();
 };
 
 class SignalEdit : public QWidget {
@@ -27,38 +32,35 @@ class SignalEdit : public QWidget {
 
 public:
   SignalEdit(int index, QWidget *parent = nullptr);
-  void setSignal(const QString &msg_id, const Signal *sig, bool show_form);
+  void setSignal(const QString &msg_id, const Signal *sig);
   void setChartOpened(bool opened);
-  void setFormVisible(bool show);
   void signalHovered(const Signal *sig);
-  inline bool isFormVisible() const { return form_container->isVisible(); }
   const Signal *sig = nullptr;
   QString msg_id;
 
 signals:
   void highlight(const Signal *sig);
   void showChart(const QString &name, const Signal *sig, bool show);
-  void showFormClicked();
   void remove(const Signal *sig);
   void save(const Signal *sig, const Signal &new_sig);
 
 protected:
+  void hideEvent(QHideEvent *event) override;
   void enterEvent(QEvent *event) override;
   void leaveEvent(QEvent *event) override;
   void saveSignal();
+  void updateForm(bool show);
+  void showFormClicked();
 
   SignalForm *form = nullptr;
   ElidedLabel *title;
-  QWidget *form_container;
   QLabel *icon;
   int form_idx = 0;
   bool chart_opened = false;
-  QPushButton *plot_btn;
+  QAction *plot_btn;
 };
 
 class SignalFindDlg : public QDialog {
-  Q_OBJECT
-
 public:
   SignalFindDlg(const QString &id, const Signal *signal, QWidget *parent);
 };

tools/cabana/tests/test_cabana.cc

@@ -5,10 +5,8 @@
 TEST_CASE("DBCManager::generateDBC") {
   DBCManager dbc_origin(nullptr);
   dbc_origin.open("toyota_new_mc_pt_generated");
-  QString dbc_string = dbc_origin.generateDBC();
-
   DBCManager dbc_from_generated(nullptr);
-  dbc_from_generated.open("", dbc_string);
+  dbc_from_generated.open("", dbc_origin.generateDBC());
 
   auto &msgs = dbc_origin.messages();
   auto &new_msgs = dbc_from_generated.messages();
@@ -18,17 +16,7 @@ TEST_CASE("DBCManager::generateDBC") {
     REQUIRE(m.name == new_m.name);
     REQUIRE(m.size == new_m.size);
     REQUIRE(m.sigs.size() == new_m.sigs.size());
-    for (auto &[name, sig] : m.sigs) {
+    for (auto &[name, sig] : m.sigs)
-      auto &new_sig = new_m.sigs[name];
+      REQUIRE(sig == new_m.sigs[name]);
-      REQUIRE(sig.name == new_sig.name);
-      REQUIRE(sig.start_bit == new_sig.start_bit);
-      REQUIRE(sig.msb == new_sig.msb);
-      REQUIRE(sig.lsb == new_sig.lsb);
-      REQUIRE(sig.size == new_sig.size);
-      REQUIRE(sig.is_signed == new_sig.is_signed);
-      REQUIRE(sig.factor == new_sig.factor);
-      REQUIRE(sig.offset == new_sig.offset);
-      REQUIRE(sig.is_little_endian == new_sig.is_little_endian);
-    }
   }
 }