From: Soeren Apel <redacted>
Date: Sat, 22 Aug 2020 20:49:16 +0000 (+0200)
Subject: Make error handling generic improve math error detail
X-Git-Url: https://sigrok.org/gitweb/?a=commitdiff_plain;ds=sidebyside;h=f6a93932056dab5e2f75207b65197b436d4141a5;hp=144e72c9ec677e7df35d37d7de6e8a18bb3f2ba1;p=pulseview.git

Make error handling generic improve math error detail
---

diff --git a/pv/data/decodesignal.cpp b/pv/data/decodesignal.cpp
index 256c29b8..b665ba89 100644
--- a/pv/data/decodesignal.cpp
+++ b/pv/data/decodesignal.cpp
@@ -56,8 +56,7 @@ DecodeSignal::DecodeSignal(pv::Session &session) :
 	srd_session_(nullptr),
 	logic_mux_data_invalid_(false),
 	stack_config_changed_(true),
-	current_segment_id_(0),
-	error_message_("")
+	current_segment_id_(0)
 {
 	connect(&session_, SIGNAL(capture_state_changed(int)),
 		this, SLOT(on_capture_state_changed(int)));
@@ -266,12 +265,6 @@ bool DecodeSignal::is_paused() const
 	return decode_paused_;
 }
 
-QString DecodeSignal::error_message() const
-{
-	lock_guard<mutex> lock(output_mutex_);
-	return error_message_;
-}
-
 const vector<decode::DecodeChannel> DecodeSignal::get_channels() const
 {
 	return channels_;
@@ -829,13 +822,6 @@ void DecodeSignal::restore_settings(QSettings &settings)
 	begin_decode();
 }
 
-void DecodeSignal::set_error_message(QString msg)
-{
-	error_message_ = msg;
-	// TODO Emulate noquote()
-	qDebug().nospace() << name() << ": " << msg;
-}
-
 bool DecodeSignal::all_input_segments_complete(uint32_t segment_id) const
 {
 	bool all_complete = true;
diff --git a/pv/data/decodesignal.hpp b/pv/data/decodesignal.hpp
index bfbf5ee0..c9ebb2a6 100644
--- a/pv/data/decodesignal.hpp
+++ b/pv/data/decodesignal.hpp
@@ -27,7 +27,6 @@
 #include <vector>
 
 #include <QSettings>
-#include <QString>
 
 #include <libsigrokdecode/libsigrokdecode.h>
 
@@ -115,7 +114,6 @@ public:
 	void pause_decode();
 	void resume_decode();
 	bool is_paused() const;
-	QString error_message() const;
 
 	const vector<decode::DecodeChannel> get_channels() const;
 	void auto_assign_signals(const shared_ptr<Decoder> dec);
@@ -189,8 +187,6 @@ public:
 	virtual void restore_settings(QSettings &settings);
 
 private:
-	void set_error_message(QString msg);
-
 	bool all_input_segments_complete(uint32_t segment_id) const;
 	uint32_t get_input_segment_count() const;
 	double get_input_samplerate(uint32_t segment_id) const;
@@ -261,8 +257,6 @@ private:
 	atomic<bool> decode_interrupt_, logic_mux_interrupt_;
 
 	bool decode_paused_;
-
-	QString error_message_;
 };
 
 } // namespace data
diff --git a/pv/data/mathsignal.cpp b/pv/data/mathsignal.cpp
index 2ad90212..e560a2e7 100644
--- a/pv/data/mathsignal.cpp
+++ b/pv/data/mathsignal.cpp
@@ -84,7 +84,6 @@ MathSignal::MathSignal(pv::Session &session) :
 	use_custom_sample_rate_(false),
 	use_custom_sample_count_(false),
 	expression_(""),
-	error_message_(""),
 	exprtk_unknown_symbol_table_(nullptr),
 	exprtk_symbol_table_(nullptr),
 	exprtk_expression_(nullptr),
@@ -137,11 +136,6 @@ void MathSignal::restore_settings(QSettings &settings)
 		use_custom_sample_count_ = settings.value("use_custom_sample_count").toBool();
 }
 
-QString MathSignal::error_message() const
-{
-	return error_message_;
-}
-
 QString MathSignal::get_expression() const
 {
 	return expression_;
@@ -159,6 +153,8 @@ void MathSignal::set_error_message(QString msg)
 	error_message_ = msg;
 	// TODO Emulate noquote()
 	qDebug().nospace() << name() << ": " << msg << "(Expression: '" << expression_ << "')";
+
+	error_message_changed(msg);
 }
 
 uint64_t MathSignal::get_working_sample_count(uint32_t segment_id) const
@@ -313,7 +309,25 @@ void MathSignal::begin_generation()
 	exprtk_parser_->enable_unknown_symbol_resolver();
 
 	if (!exprtk_parser_->compile(expression_.toStdString(), *exprtk_expression_)) {
-		set_error_message(tr("Error in expression"));
+		QString error_details;
+		size_t error_count = exprtk_parser_->error_count();
+
+		for (size_t i = 0; i < error_count; i++) {
+			typedef exprtk::parser_error::type error_t;
+			error_t error = exprtk_parser_->get_error(i);
+			exprtk::parser_error::update_error(error, expression_.toStdString());
+
+			QString error_detail = tr("%1 at line %2, column %3: %4");
+			if ((error_count > 1) && (i < (error_count - 1)))
+				error_detail += "\n";
+
+			error_details += error_detail \
+				.arg(exprtk::parser_error::to_str(error.mode).c_str()) \
+				.arg(error.line_no) \
+				.arg(error.column_no) \
+				.arg(error.diagnostic.c_str());
+		}
+		set_error_message(error_details);
 	} else {
 		// Resolve unknown scalars to signals and add them to the input signal list
 		vector<string> unknowns;
diff --git a/pv/data/mathsignal.hpp b/pv/data/mathsignal.hpp
index 6127b177..67cc5068 100644
--- a/pv/data/mathsignal.hpp
+++ b/pv/data/mathsignal.hpp
@@ -60,7 +60,6 @@ class MathSignal : public SignalBase
 {
 	Q_OBJECT
 	Q_PROPERTY(QString expression READ get_expression WRITE set_expression NOTIFY expression_changed)
-	Q_PROPERTY(QString error_message READ error_message)
 
 private:
 	static const int64_t ChunkLength;
@@ -72,13 +71,11 @@ public:
 	virtual void save_settings(QSettings &settings) const;
 	virtual void restore_settings(QSettings &settings);
 
-	QString error_message() const;
-
 	QString get_expression() const;
 	void set_expression(QString expression);
 
 private:
-	void set_error_message(QString msg);
+	virtual void set_error_message(QString msg);
 
 	/**
 	 * Returns the number of samples that can be worked on,
@@ -123,7 +120,7 @@ private:
 	bool use_custom_sample_rate_, use_custom_sample_count_;
 	map<std::string, signal_data> input_signals_;
 
-	QString expression_, error_message_;
+	QString expression_;
 
 	mutable mutex input_mutex_;
 	mutable condition_variable gen_input_cond_;
diff --git a/pv/data/signalbase.cpp b/pv/data/signalbase.cpp
index 104243b1..94e1f6a6 100644
--- a/pv/data/signalbase.cpp
+++ b/pv/data/signalbase.cpp
@@ -123,7 +123,8 @@ SignalBase::SignalBase(shared_ptr<sigrok::Channel> channel, ChannelType channel_
 	group_(nullptr),
 	conversion_type_(NoConversion),
 	min_value_(0),
-	max_value_(0)
+	max_value_(0),
+	error_message_("")
 {
 	if (channel_) {
 		internal_name_ = QString::fromStdString(channel_->name());
@@ -258,6 +259,11 @@ QColor SignalBase::bgcolor() const
 	return bgcolor_;
 }
 
+QString SignalBase::get_error_message() const
+{
+	return error_message_;
+}
+
 void SignalBase::set_data(shared_ptr<pv::data::SignalData> data)
 {
 	if (data_) {
@@ -824,6 +830,15 @@ void SignalBase::start_conversion(bool delayed_start)
 	conversion_thread_ = std::thread(&SignalBase::conversion_thread_proc, this);
 }
 
+void SignalBase::set_error_message(QString msg)
+{
+	error_message_ = msg;
+	// TODO Emulate noquote()
+	qDebug().nospace() << name() << ": " << msg;
+
+	error_message_changed(msg);
+}
+
 void SignalBase::stop_conversion()
 {
 	// Stop conversion so we can restart it from the beginning
diff --git a/pv/data/signalbase.hpp b/pv/data/signalbase.hpp
index 89bfc252..a55b3fc2 100644
--- a/pv/data/signalbase.hpp
+++ b/pv/data/signalbase.hpp
@@ -87,6 +87,7 @@ private:
 class SignalBase : public QObject, public enable_shared_from_this<SignalBase>
 {
 	Q_OBJECT
+	Q_PROPERTY(QString error_message READ get_error_message)
 
 public:
 	enum ChannelType {
@@ -225,6 +226,11 @@ public:
 	 */
 	QColor bgcolor() const;
 
+	/**
+	 * Returns the current error message text.
+	 */
+	virtual QString get_error_message() const;
+
 	/**
 	 * Sets the internal data object.
 	 */
@@ -346,7 +352,12 @@ public:
 
 	void start_conversion(bool delayed_start=false);
 
+protected:
+	virtual void set_error_message(QString msg);
+
 private:
+	void stop_conversion();
+
 	bool conversion_is_a2l() const;
 
 	uint8_t convert_a2l_threshold(float threshold, float value);
@@ -359,19 +370,14 @@ private:
 		shared_ptr<LogicSegment> lsegment);
 	void conversion_thread_proc();
 
-	void stop_conversion();
-
 Q_SIGNALS:
 	void enabled_changed(const bool &value);
-
 	void name_changed(const QString &name);
-
 	void color_changed(const QColor &color);
-
+	void error_message_changed(const QString &msg);
 	void conversion_type_changed(const ConversionType t);
 
 	void samples_cleared();
-
 	void samples_added(uint64_t segment_id, uint64_t start_sample,
 		uint64_t end_sample);
 
@@ -409,6 +415,8 @@ protected:
 	QString internal_name_, name_;
 	QColor color_, bgcolor_;
 	unsigned int index_;
+
+	QString error_message_;
 };
 
 } // namespace data
diff --git a/pv/views/trace/analogsignal.cpp b/pv/views/trace/analogsignal.cpp
index ea33823d..e6b6d496 100644
--- a/pv/views/trace/analogsignal.cpp
+++ b/pv/views/trace/analogsignal.cpp
@@ -257,32 +257,36 @@ void AnalogSignal::paint_mid(QPainter &p, ViewItemPaintParams &pp)
 		paint_grid(p, y, pp.left(), pp.right());
 
 		shared_ptr<pv::data::AnalogSegment> segment = get_analog_segment_to_paint();
-		if (!segment || (segment->get_sample_count() == 0))
-			return;
-
-		const double pixels_offset = pp.pixels_offset();
-		const double samplerate = max(1.0, segment->samplerate());
-		const pv::util::Timestamp& start_time = segment->start_time();
-		const int64_t last_sample = (int64_t)segment->get_sample_count() - 1;
-		const double samples_per_pixel = samplerate * pp.scale();
-		const pv::util::Timestamp start = samplerate * (pp.offset() - start_time);
-		const pv::util::Timestamp end = start + samples_per_pixel * pp.width();
-
-		const int64_t start_sample = min(max(floor(start).convert_to<int64_t>(),
-			(int64_t)0), last_sample);
-		const int64_t end_sample = min(max((ceil(end) + 1).convert_to<int64_t>(),
-			(int64_t)0), last_sample);
-
-		if (samples_per_pixel < EnvelopeThreshold)
-			paint_trace(p, segment, y, pp.left(), start_sample, end_sample,
-				pixels_offset, samples_per_pixel);
-		else
-			paint_envelope(p, segment, y, pp.left(), start_sample, end_sample,
-				pixels_offset, samples_per_pixel);
+
+		if (segment && (segment->get_sample_count() > 0)) {
+			const double pixels_offset = pp.pixels_offset();
+			const double samplerate = max(1.0, segment->samplerate());
+			const pv::util::Timestamp& start_time = segment->start_time();
+			const int64_t last_sample = (int64_t)segment->get_sample_count() - 1;
+			const double samples_per_pixel = samplerate * pp.scale();
+			const pv::util::Timestamp start = samplerate * (pp.offset() - start_time);
+			const pv::util::Timestamp end = start + samples_per_pixel * pp.width();
+
+			const int64_t start_sample = min(max(floor(start).convert_to<int64_t>(),
+				(int64_t)0), last_sample);
+			const int64_t end_sample = min(max((ceil(end) + 1).convert_to<int64_t>(),
+				(int64_t)0), last_sample);
+
+			if (samples_per_pixel < EnvelopeThreshold)
+				paint_trace(p, segment, y, pp.left(), start_sample, end_sample,
+					pixels_offset, samples_per_pixel);
+			else
+				paint_envelope(p, segment, y, pp.left(), start_sample, end_sample,
+					pixels_offset, samples_per_pixel);
+		}
 	}
 
 	if ((display_type_ == DisplayConverted) || (display_type_ == DisplayBoth))
 		paint_logic_mid(p, pp);
+
+	const QString err = base_->get_error_message();
+	if (!err.isEmpty())
+		paint_error(p, pp);
 }
 
 void AnalogSignal::paint_fore(QPainter &p, ViewItemPaintParams &pp)
diff --git a/pv/views/trace/decodetrace.cpp b/pv/views/trace/decodetrace.cpp
index 93c7c5a9..2902c936 100644
--- a/pv/views/trace/decodetrace.cpp
+++ b/pv/views/trace/decodetrace.cpp
@@ -82,7 +82,6 @@ namespace pv {
 namespace views {
 namespace trace {
 
-const QColor DecodeTrace::ErrorBgColor = QColor(0xEF, 0x29, 0x29);
 const QColor DecodeTrace::NoDecodeColor = QColor(0x88, 0x8A, 0x85);
 const QColor DecodeTrace::ExpandMarkerWarnColor = QColor(0xFF, 0xA5, 0x00); // QColorConstants::Svg::orange
 const QColor DecodeTrace::ExpandMarkerHiddenColor = QColor(0x69, 0x69, 0x69); // QColorConstants::Svg::dimgray
@@ -337,9 +336,9 @@ void DecodeTrace::paint_mid(QPainter &p, ViewItemPaintParams &pp)
 		owner_->row_item_appearance_changed(false, true);
 	}
 
-	const QString err = decode_signal_->error_message();
+	const QString err = base_->get_error_message();
 	if (!err.isEmpty())
-		draw_error(p, err, pp);
+		paint_error(p, pp);
 
 #if DECODETRACE_SHOW_RENDER_TIME
 	qDebug() << "Rendering" << base_->name() << "took" << render_time_.elapsed() << "ms";
@@ -908,28 +907,6 @@ void DecodeTrace::draw_range(const Annotation* a, QPainter &p,
 		best_annotation, Qt::ElideRight, rect.width()));
 }
 
-void DecodeTrace::draw_error(QPainter &p, const QString &message,
-	const ViewItemPaintParams &pp)
-{
-	const int y = get_visual_y();
-
-	double samples_per_pixel, pixels_offset;
-	tie(pixels_offset, samples_per_pixel) = get_pixels_offset_samples_per_pixel();
-
-	p.setPen(ErrorBgColor.darker());
-	p.setBrush(ErrorBgColor);
-
-	const QRectF bounding_rect = QRectF(pp.left(), INT_MIN / 2 + y, pp.right(), INT_MAX);
-
-	const QRectF text_rect = p.boundingRect(bounding_rect, Qt::AlignCenter, message);
-	const qreal r = text_rect.height() / 4;
-
-	p.drawRoundedRect(text_rect.adjusted(-r, -r, r, r), r, r, Qt::AbsoluteSize);
-
-	p.setPen(Qt::black);
-	p.drawText(text_rect, message);
-}
-
 void DecodeTrace::draw_unresolved_period(QPainter &p, int left, int right) const
 {
 	double samples_per_pixel, pixels_offset;
diff --git a/pv/views/trace/decodetrace.hpp b/pv/views/trace/decodetrace.hpp
index 1b79161d..80c9cf75 100644
--- a/pv/views/trace/decodetrace.hpp
+++ b/pv/views/trace/decodetrace.hpp
@@ -120,7 +120,6 @@ class DecodeTrace : public Trace
 	Q_OBJECT
 
 private:
-	static const QColor ErrorBgColor;
 	static const QColor NoDecodeColor;
 	static const QColor ExpandMarkerWarnColor;
 	static const QColor ExpandMarkerHiddenColor;
diff --git a/pv/views/trace/trace.cpp b/pv/views/trace/trace.cpp
index ecc1aecc..1f77598d 100644
--- a/pv/views/trace/trace.cpp
+++ b/pv/views/trace/trace.cpp
@@ -49,6 +49,7 @@ const int Trace::LabelHitPadding = 2;
 
 const QColor Trace::BrightGrayBGColor = QColor(0, 0, 0, 10 * 255 / 100);
 const QColor Trace::DarkGrayBGColor = QColor(0, 0, 0, 15 * 255 / 100);
+const QColor Trace::ErrorBgColor = QColor(0xEF, 0x29, 0x29);
 
 Trace::Trace(shared_ptr<data::SignalBase> signal) :
 	base_(signal),
@@ -62,6 +63,8 @@ Trace::Trace(shared_ptr<data::SignalBase> signal) :
 		this, SLOT(on_name_changed(const QString&)));
 	connect(signal.get(), SIGNAL(color_changed(const QColor&)),
 		this, SLOT(on_color_changed(const QColor&)));
+	connect(signal.get(), SIGNAL(error_message_changed(const QString&)),
+		this, SLOT(on_error_message_changed(const QString&)));
 
 	GlobalSettings::add_change_handler(this);
 
@@ -175,6 +178,26 @@ void Trace::paint_label(QPainter &p, const QRect &rect, bool hover)
 		Qt::AlignCenter | Qt::AlignVCenter, base_->name());
 }
 
+void Trace::paint_error(QPainter &p, const ViewItemPaintParams &pp)
+{
+	const QString message = base_->get_error_message();
+
+	const int y = get_visual_y();
+
+	p.setPen(ErrorBgColor.darker());
+	p.setBrush(ErrorBgColor);
+
+	const QRectF bounding_rect = QRectF(pp.left(), INT_MIN / 2 + y, pp.right(), INT_MAX);
+
+	const QRectF text_rect = p.boundingRect(bounding_rect, Qt::AlignCenter, message);
+	const qreal r = text_rect.height() / 4;
+
+	p.drawRoundedRect(text_rect.adjusted(-r, -r, r, r), r, r, Qt::AbsoluteSize);
+
+	p.setPen(Qt::black);
+	p.drawText(text_rect, message);
+}
+
 QMenu* Trace::create_header_context_menu(QWidget *parent)
 {
 	QMenu *const menu = ViewItem::create_header_context_menu(parent);
@@ -402,6 +425,14 @@ void Trace::on_color_changed(const QColor &color)
 		owner_->row_item_appearance_changed(true, true);
 }
 
+void Trace::on_error_message_changed(const QString &msg)
+{
+	(void)msg;
+
+	if (owner_)
+		owner_->row_item_appearance_changed(false, true);
+}
+
 void Trace::on_popup_closed()
 {
 	popup_ = nullptr;
diff --git a/pv/views/trace/trace.hpp b/pv/views/trace/trace.hpp
index 05be7053..741f3469 100644
--- a/pv/views/trace/trace.hpp
+++ b/pv/views/trace/trace.hpp
@@ -91,6 +91,7 @@ private:
 
 	static const QColor BrightGrayBGColor;
 	static const QColor DarkGrayBGColor;
+	static const QColor ErrorBgColor;
 
 protected:
 	Trace(shared_ptr<data::SignalBase> signal);
@@ -127,6 +128,13 @@ public:
 	 */
 	virtual void paint_label(QPainter &p, const QRect &rect, bool hover);
 
+	/**
+	 * Paints the signal's current error message text.
+	 * @param p the QPainter to paint into.
+	 * @param pp The painting parameters object to paint with.
+	 */
+	virtual void paint_error(QPainter &p, const ViewItemPaintParams &pp);
+
 	virtual QMenu* create_header_context_menu(QWidget *parent);
 
 	virtual QMenu* create_view_context_menu(QWidget *parent, QPoint &click_pos);
@@ -142,7 +150,7 @@ public:
 
 	/**
 	 * Computes the outline rectangle of the viewport hit-box.
-	 * @param rect the rectangle of the viewport area.
+	 * @param pp The painting parameters object to paint with.
 	 * @return Returns the rectangle of the hit-box.
 	 * @remarks The default implementation returns an empty hit-box.
 	 */
@@ -184,8 +192,8 @@ protected:
 
 protected Q_SLOTS:
 	virtual void on_name_changed(const QString &text);
-
 	virtual void on_color_changed(const QColor &color);
+	virtual void on_error_message_changed(const QString &msg);
 
 	void on_popup_closed();