Cabana: no popup when adding a new signal. (#26155)

* merge master

* GM: make loopback updated check more explicit

This previously checked if parser_pyx filled the defaultdict, now it checks the values in the list (same either way)

* merge master

* continue

* continue

* check for duplicate name

* check duplicate signal name in saveSignal

* check signal size

* size*8

* cleanup

* check size after push_back

* install qlog handler at end

* remove todo

Co-authored-by: Shane Smiskol <shane@smiskol.com>

tools/cabana/binaryview.cc

@@ -13,12 +13,6 @@
 
 const int CELL_HEIGHT = 30;
 
-static std::pair<int, int> getSignalRange(const Signal *s) {
-  int from = s->is_little_endian ? s->start_bit : bigEndianBitIndex(s->start_bit);
-  int to = from + s->size - 1;
-  return {from, to};
-}
-
 BinaryView::BinaryView(QWidget *parent) : QTableView(parent) {
   model = new BinaryViewModel(this);
   setModel(model);

tools/cabana/canmessages.cc

@@ -54,9 +54,9 @@ QList<QPointF> CANMessages::findSignalValues(const QString &id, const Signal *si
         double val = get_raw_value((uint8_t *)c.getDat().begin(), c.getDat().size(), *signal);
         if ((flag == EQ && val == value) || (flag == LT && val < value) || (flag == GT && val > value)) {
           ret.push_back({(evt->mono_time / (double)1e9) - can->routeStartTime(), val});
+          if (ret.size() >= max_count)
+            return ret;
         }
-        if (ret.size() >= max_count)
-          return ret;
       }
     }
   }

tools/cabana/dbcmanager.cc

@@ -121,3 +121,21 @@ double get_raw_value(uint8_t *data, size_t data_size, const Signal &sig) {
   double value = val * sig.factor + sig.offset;
   return value;
 }
+
+void updateSigSizeParamsFromRange(Signal &s, int from, int to) {
+  s.start_bit = s.is_little_endian ? from : bigEndianBitIndex(from);
+  s.size = to - from + 1;
+  if (s.is_little_endian) {
+    s.lsb = s.start_bit;
+    s.msb = s.start_bit + s.size - 1;
+  } else {
+    s.lsb = bigEndianStartBitsIndex(bigEndianBitIndex(s.start_bit) + s.size - 1);
+    s.msb = s.start_bit;
+  }
+}
+
+std::pair<int, int> getSignalRange(const Signal *s) {
+  int from = s->is_little_endian ? s->start_bit : bigEndianBitIndex(s->start_bit);
+  int to = from + s->size - 1;
+  return {from, to};
+}

tools/cabana/dbcmanager.h

@@ -47,5 +47,7 @@ private:
 double get_raw_value(uint8_t *data, size_t data_size, const Signal &sig);
 int bigEndianStartBitsIndex(int start_bit);
 int bigEndianBitIndex(int index);
+void updateSigSizeParamsFromRange(Signal &s, int from, int to);
+std::pair<int, int> getSignalRange(const Signal *s);
 
 DBCManager *dbc();

tools/cabana/detailwidget.cc

@@ -48,7 +48,7 @@ DetailWidget::DetailWidget(QWidget *parent) : QWidget(parent) {
   warning_widget = new QWidget(this);
   QHBoxLayout *warning_hlayout = new QHBoxLayout(warning_widget);
   QLabel *warning_icon = new QLabel(this);
-  warning_icon->setPixmap(style()->standardIcon(QStyle::SP_MessageBoxWarning).pixmap({16, 16}));
+  warning_icon->setPixmap(style()->standardPixmap(QStyle::SP_MessageBoxWarning));
   warning_hlayout->addWidget(warning_icon);
   warning_label = new QLabel(this);
   warning_hlayout->addWidget(warning_label, 1, Qt::AlignLeft);
@@ -78,7 +78,7 @@ DetailWidget::DetailWidget(QWidget *parent) : QWidget(parent) {
   QObject::connect(binary_view, &BinaryView::resizeSignal, this, &DetailWidget::resizeSignal);
   QObject::connect(binary_view, &BinaryView::addSignal, this, &DetailWidget::addSignal);
   QObject::connect(can, &CANMessages::updated, this, &DetailWidget::updateState);
-  QObject::connect(dbc(), &DBCManager::DBCFileChanged, this, &DetailWidget::dbcMsgChanged);
+  QObject::connect(dbc(), &DBCManager::DBCFileChanged, [this]() { dbcMsgChanged(); });
   QObject::connect(tabbar, &QTabBar::currentChanged, [this](int index) { setMessage(messages[index]); });
   QObject::connect(tabbar, &QTabBar::tabCloseRequested, [=](int index) {
     messages.removeAt(index);
@@ -103,7 +103,7 @@ void DetailWidget::setMessage(const QString &message_id) {
   dbcMsgChanged();
 }
 
-void DetailWidget::dbcMsgChanged() {
+void DetailWidget::dbcMsgChanged(int show_form_idx) {
   if (msg_id.isEmpty()) return;
 
   warning_widget->hide();
@@ -111,7 +111,7 @@ void DetailWidget::dbcMsgChanged() {
   QString msg_name = tr("untitled");
   if (auto msg = dbc()->msg(msg_id)) {
     for (int i = 0; i < msg->sigs.size(); ++i) {
-      auto form = new SignalEdit(i, msg_id, msg->sigs[i]);
+      auto form = new SignalEdit(i, msg_id, &(msg->sigs[i]));
       signals_container->layout()->addWidget(form);
       QObject::connect(form, &SignalEdit::showChart, [this, sig = &msg->sigs[i]]() { emit showChart(msg_id, sig); });
       QObject::connect(form, &SignalEdit::showFormClicked, this, &DetailWidget::showForm);
@@ -119,6 +119,9 @@ void DetailWidget::dbcMsgChanged() {
       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);
+      if (i == show_form_idx) {
+        QTimer::singleShot(0, [=]() { emit form->showFormClicked(); });
+      }
     }
     msg_name = msg->name.c_str();
     if (msg->size != can->lastMessage(msg_id).dat.size()) {
@@ -162,46 +165,54 @@ void DetailWidget::editMsg() {
   }
 }
 
-void DetailWidget::addSignal(int start_bit, int to) {
+void DetailWidget::addSignal(int from, int to) {
-  if (dbc()->msg(msg_id)) {
+  if (auto msg = dbc()->msg(msg_id)) {
-    AddSignalDialog dlg(msg_id, start_bit, to - start_bit + 1, this);
+    Signal sig = {};
-    if (dlg.exec()) {
+    for (int i = 1; /**/; ++i) {
-      dbc()->addSignal(msg_id, dlg.form->getSignal());
+      sig.name = "NEW_SIGNAL_" + std::to_string(i);
-      dbcMsgChanged();
+      auto it = std::find_if(msg->sigs.begin(), msg->sigs.end(), [&](auto &s) { return sig.name == s.name; });
+      if (it == msg->sigs.end()) break;
     }
+    sig.is_little_endian = false,
+    updateSigSizeParamsFromRange(sig, from, to);
+    dbc()->addSignal(msg_id, sig);
+    dbcMsgChanged(msg->sigs.size() - 1);
   }
 }
 
 void DetailWidget::resizeSignal(const Signal *sig, int from, int to) {
-  assert(sig != nullptr);
   Signal s = *sig;
-  s.start_bit = s.is_little_endian ? from : bigEndianBitIndex(from);;
+  updateSigSizeParamsFromRange(s, from, to);
-  s.size = to - from + 1;
+  saveSignal(sig, s);
-  if (s.is_little_endian) {
-    s.lsb = s.start_bit;
-    s.msb = s.start_bit + s.size - 1;
-  } else {
-    s.lsb = bigEndianStartBitsIndex(bigEndianBitIndex(s.start_bit) + s.size - 1);
-    s.msb = s.start_bit;
-  }
-  dbc()->updateSignal(msg_id, s.name.c_str(), s);
-  dbcMsgChanged();
 }
 
-void DetailWidget::saveSignal() {
+void DetailWidget::saveSignal(const Signal *sig, const Signal &new_sig) {
-  SignalEdit *sig_form = qobject_cast<SignalEdit *>(QObject::sender());
+  auto msg = dbc()->msg(msg_id);
-  auto s = sig_form->form->getSignal();
+  if (new_sig.name != sig->name) {
-  dbc()->updateSignal(msg_id, sig_form->sig_name, s);
+    auto it = std::find_if(msg->sigs.begin(), msg->sigs.end(), [&](auto &s) { return s.name == new_sig.name; });
+    if (it != msg->sigs.end()) {
+      QString warning_str = tr("There is already a signal with the same name '%1'").arg(new_sig.name.c_str());
+      QMessageBox::warning(this, tr("Failed to save signal"), warning_str);
+      return;
+    }
+  }
+
+  auto [start, end] = getSignalRange(&new_sig);
+  if (start < 0 || end >= msg->size * 8) {
+    QString warning_str = tr("Signal size [%1] exceed limit").arg(new_sig.size);
+    QMessageBox::warning(this, tr("Failed to save signal"), warning_str);
+    return;
+  }
+
+  dbc()->updateSignal(msg_id, sig->name.c_str(), new_sig);
   // update binary view and history log
-  binary_view->setMessage(msg_id);
+  dbcMsgChanged();
-  history_log->setMessage(msg_id);
 }
 
-void DetailWidget::removeSignal() {
+void DetailWidget::removeSignal(const Signal *sig) {
-  SignalEdit *sig_form = qobject_cast<SignalEdit *>(QObject::sender());
+  QString text = tr("Are you sure you want to remove signal '%1'").arg(sig->name.c_str());
-  QString text = tr("Are you sure you want to remove signal '%1'").arg(sig_form->sig_name);
   if (QMessageBox::Yes == QMessageBox::question(this, tr("Remove signal"), text)) {
-    dbc()->removeSignal(msg_id, sig_form->sig_name);
+    dbc()->removeSignal(msg_id, sig->name.c_str());
     dbcMsgChanged();
   }
 }

tools/cabana/detailwidget.h

@@ -32,7 +32,7 @@ class DetailWidget : public QWidget {
 public:
   DetailWidget(QWidget *parent);
   void setMessage(const QString &message_id);
-  void dbcMsgChanged();
+  void dbcMsgChanged(int show_form_idx = -1);
 
 signals:
   void showChart(const QString &msg_id, const Signal *sig);
@@ -41,8 +41,8 @@ signals:
 private:
   void addSignal(int start_bit, int to);
   void resizeSignal(const Signal *sig, int from, int to);
-  void saveSignal();
+  void saveSignal(const Signal *sig, const Signal &new_sig);
-  void removeSignal();
+  void removeSignal(const Signal *sig);
   void editMsg();
   void showForm();
   void updateState();

tools/cabana/mainwin.cc

@@ -14,9 +14,6 @@ void qLogMessageHandler(QtMsgType type, const QMessageLogContext &context, const
 }
 
 MainWindow::MainWindow() : QWidget() {
-  main_win = this;
-  qInstallMessageHandler(qLogMessageHandler);
-
   QVBoxLayout *main_layout = new QVBoxLayout(this);
   main_layout->setContentsMargins(11, 11, 11, 5);
   main_layout->setSpacing(0);
@@ -88,6 +85,9 @@ MainWindow::MainWindow() : QWidget() {
   QObject::connect(charts_widget, &ChartsWidget::dock, this, &MainWindow::dockCharts);
   QObject::connect(settings_btn, &QPushButton::clicked, this, &MainWindow::setOption);
   QObject::connect(can, &CANMessages::eventsMerged, [=]() { fingerprint_label->setText(can->carFingerprint() ); });
+
+  main_win = this;
+  qInstallMessageHandler(qLogMessageHandler);
 }
 
 void MainWindow::updateDownloadProgress(uint64_t cur, uint64_t total, bool success) {

tools/cabana/signaledit.cc

@@ -3,6 +3,7 @@
 #include <QDialogButtonBox>
 #include <QFormLayout>
 #include <QHBoxLayout>
+#include <QMessageBox>
 #include <QRadioButton>
 #include <QScrollArea>
 #include <QVBoxLayout>
@@ -11,7 +12,7 @@
 
 // SignalForm
 
-SignalForm::SignalForm(const Signal &sig, QWidget *parent) : start_bit(sig.start_bit), QWidget(parent) {
+SignalForm::SignalForm(const Signal &sig, QWidget *parent) : QWidget(parent) {
   QFormLayout *form_layout = new QFormLayout(this);
 
   name = new QLineEdit(sig.name.c_str());
@@ -59,30 +60,9 @@ SignalForm::SignalForm(const Signal &sig, QWidget *parent) : start_bit(sig.start
   form_layout->addRow(tr("Value descriptions"), val_desc);
 }
 
-Signal SignalForm::getSignal() {
-  // TODO: Check if the size is valid, and no duplicate name
-  Signal sig = {};
-  sig.start_bit = start_bit;
-  sig.name = name->text().toStdString();
-  sig.size = size->text().toInt();
-  sig.offset = offset->text().toDouble();
-  sig.factor = factor->text().toDouble();
-  sig.is_signed = sign->currentIndex() == 0;
-  sig.is_little_endian = endianness->currentIndex() == 0;
-  if (sig.is_little_endian) {
-    sig.lsb = sig.start_bit;
-    sig.msb = sig.start_bit + sig.size - 1;
-  } else {
-    sig.lsb = bigEndianStartBitsIndex(bigEndianBitIndex(sig.start_bit) + sig.size - 1);
-    sig.msb = sig.start_bit;
-  }
-  return sig;
-}
-
 // SignalEdit
 
-SignalEdit::SignalEdit(int index, const QString &msg_id, const Signal &sig, QWidget *parent)
+SignalEdit::SignalEdit(int index, const QString &msg_id, const Signal *sig, QWidget *parent) : msg_id(msg_id), sig(sig), form_idx(index), QWidget(parent) {
-    : sig(&sig), form_idx(index), sig_name(sig.name.c_str()), QWidget(parent) {
   QVBoxLayout *main_layout = new QVBoxLayout(this);
   main_layout->setContentsMargins(0, 0, 0, 0);
 
@@ -93,7 +73,7 @@ SignalEdit::SignalEdit(int index, const QString &msg_id, const Signal &sig, QWid
   title_layout->addWidget(icon);
   title = new ElidedLabel(this);
   title->setSizePolicy(QSizePolicy::Preferred, QSizePolicy::MinimumExpanding);
-  title->setText(QString("%1. %2").arg(index + 1).arg(sig_name));
+  title->setText(QString("%1. %2").arg(index + 1).arg(sig->name.c_str()));
   title->setStyleSheet(QString("font-weight:bold; color:%1").arg(getColor(index)));
   title_layout->addWidget(title, 1);
 
@@ -113,7 +93,7 @@ SignalEdit::SignalEdit(int index, const QString &msg_id, const Signal &sig, QWid
   // signal form
   form_container = new QWidget(this);
   QVBoxLayout *v_layout = new QVBoxLayout(form_container);
-  form = new SignalForm(sig, this);
+  form = new SignalForm(*sig, this);
   v_layout->addWidget(form);
 
   QHBoxLayout *h = new QHBoxLayout();
@@ -133,20 +113,34 @@ SignalEdit::SignalEdit(int index, const QString &msg_id, const Signal &sig, QWid
   hline->setFrameShadow(QFrame::Sunken);
   main_layout->addWidget(hline);
 
-  QObject::connect(remove_btn, &QPushButton::clicked, this, &SignalEdit::remove);
+  QObject::connect(remove_btn, &QPushButton::clicked, [this]() { emit remove(this->sig); });
   QObject::connect(title, &ElidedLabel::clicked, this, &SignalEdit::showFormClicked);
-  QObject::connect(save_btn, &QPushButton::clicked, [=]() {
+  QObject::connect(save_btn, &QPushButton::clicked, this, &SignalEdit::saveSignal);
-    QString new_name = form->getSignal().name.c_str();
+  QObject::connect(seek_btn, &QPushButton::clicked, [this, msg_id]() {
-    title->setText(QString("%1. %2").arg(index + 1).arg(new_name));
+    SignalFindDlg dlg(msg_id, this->sig, this);
-    emit save();
-    sig_name = new_name;
-  });
-  QObject::connect(seek_btn, &QPushButton::clicked, [this, msg_id, s = &sig]() {
-    SignalFindDlg dlg(msg_id, s, this);
     dlg.exec();
   });
 }
 
+void SignalEdit::saveSignal() {
+  Signal s = *sig;
+  s.name = form->name->text().toStdString();
+  s.size = form->size->text().toInt();
+  s.offset = form->offset->text().toDouble();
+  s.factor = form->factor->text().toDouble();
+  s.is_signed = form->sign->currentIndex() == 0;
+  s.is_little_endian = form->endianness->currentIndex() == 0;
+  if (s.is_little_endian) {
+    s.lsb = s.start_bit;
+    s.msb = s.start_bit + s.size - 1;
+  } else {
+    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()));
+  emit save(this->sig, s);
+}
+
 void SignalEdit::setFormVisible(bool visible) {
   form_container->setVisible(visible);
   icon->setText(visible ? "▼" : ">");
@@ -167,27 +161,7 @@ void SignalEdit::leaveEvent(QEvent *event) {
   QWidget::leaveEvent(event);
 }
 
-// AddSignalDialog
+// SignalFindDlg
-
-AddSignalDialog::AddSignalDialog(const QString &id, int start_bit, int size, QWidget *parent) : QDialog(parent) {
-  setWindowTitle(tr("Add signal to %1").arg(dbc()->msg(id)->name.c_str()));
-  QVBoxLayout *main_layout = new QVBoxLayout(this);
-
-  Signal sig = {
-    .name = "untitled",
-    .start_bit = bigEndianBitIndex(start_bit),
-    .is_little_endian = false,
-    .size = size,
-  };
-  form = new SignalForm(sig, this);
-  main_layout->addWidget(form);
-  auto buttonBox = new QDialogButtonBox(QDialogButtonBox::Ok | QDialogButtonBox::Cancel);
-  main_layout->addWidget(buttonBox);
-  setFixedWidth(parent->width() * 0.9);
-
-  connect(buttonBox, &QDialogButtonBox::rejected, this, &QDialog::reject);
-  connect(buttonBox, &QDialogButtonBox::accepted, this, &QDialog::accept);
-}
 
 SignalFindDlg::SignalFindDlg(const QString &id, const Signal *signal, QWidget *parent) : QDialog(parent) {
   setWindowTitle(tr("Find signal values"));

tools/cabana/signaledit.h

@@ -15,48 +15,41 @@
 class SignalForm : public QWidget {
 public:
   SignalForm(const Signal &sig, QWidget *parent);
-  Signal getSignal();
 
   QLineEdit *name, *unit, *comment, *val_desc;
   QSpinBox *size, *offset;
   QDoubleSpinBox *factor, *min_val, *max_val;
   QComboBox *sign, *endianness;
-  int start_bit = 0;
 };
 
 class SignalEdit : public QWidget {
   Q_OBJECT
 
 public:
-  SignalEdit(int index, const QString &msg_id, const Signal &sig, QWidget *parent = nullptr);
+  SignalEdit(int index, const QString &msg_id, const Signal *sig, QWidget *parent = nullptr);
   void setFormVisible(bool show);
   void signalHovered(const Signal *sig);
   inline bool isFormVisible() const { return form_container->isVisible(); }
-  QString sig_name;
-  SignalForm *form;
-  int form_idx = 0;
-  const Signal *sig = nullptr;
 
 signals:
   void highlight(const Signal *sig);
   void showChart();
   void showFormClicked();
-  void remove();
+  void remove(const Signal *sig);
-  void save();
+  void save(const Signal *sig, const Signal &new_sig);
 
 protected:
   void enterEvent(QEvent *event) override;
   void leaveEvent(QEvent *event) override;
+  void saveSignal();
 
+  SignalForm *form;
   ElidedLabel *title;
   QWidget *form_container;
   QLabel *icon;
-};
+  int form_idx = 0;
-
+  QString msg_id;
-class AddSignalDialog : public QDialog {
+  const Signal *sig = nullptr;
-public:
-  AddSignalDialog(const QString &id, int start_bit, int size, QWidget *parent);
-  SignalForm *form;
 };
 
 class SignalFindDlg : public QDialog {