Bug 1149 - PV doesn't set widget to reflect initial voltage_threshold value
Summary: PV doesn't set widget to reflect initial voltage_threshold value
Status: RESOLVED FIXED
Alias: None
Product: PulseView
Classification: Unclassified
Component: Other (show other bugs)
Version: unreleased development snapshot
Hardware: All All
: Normal normal
Target Milestone: ---
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-31 23:03 CEST by Soeren Apel
Modified: 2018-05-07 11:56 CEST (History)
2 users (show)



Attachments
log-File without threshold (28.77 KB, text/x-log)
2018-04-02 10:32 CEST, Jörg
Details
sr-File without threshold (6.56 KB, application/zip)
2018-04-02 10:35 CEST, Jörg
Details
pic-file without threshold (24.38 KB, image/png)
2018-04-02 10:36 CEST, Jörg
Details
log-File with threshold + edge-trigger (24.81 KB, text/x-log)
2018-04-02 10:39 CEST, Jörg
Details
pic-file with threshold + trigger (29.50 KB, image/png)
2018-04-02 10:40 CEST, Jörg
Details
pic-file threshold ComboBox (42.91 KB, image/png)
2018-05-07 11:02 CEST, Jörg
Details
BugFix OK pic-file threshold ComboBox (42.68 KB, image/png)
2018-05-07 11:56 CEST, Jörg
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Soeren Apel 2018-03-31 23:03:51 CEST
As per mail from Joerg Alpers ("Pluseview Threshold every start Zerro (Dslogic)") on the ML, there appears to be a bug in PV. I quote:

sr: [263:33.507641] hwdriver: sr_config_get(): key 30023
           (voltage_threshold) sdi 0x10c02b0 cg NULL ->
           (1.3999999999999999, 1.39 99999999999999) 

The value 1.4 is important here. So not just the OPT list of the
driver, whose values ​​are converted into widget element, where my
problem lies. It is not possible for me to find a name for this element
in the source code, as it will automatically be embarrassed depending
on the ability of the driver.

Thus, I can not find a way as with the SAMPLERATE widget (which is
definitely defined) with the set command


 auto gvar = device->device()->config_get(ConfigKey::SAMPLERATE);
 uint64_t samplerate =
       Glib::VariantBase::cast_dynamic<Glib::Variant<guint64>>(gvar).get();
 assert(!updating_sample_rate_); 
 updating_sample_rate_ = true; 
 sample_rate_.set_value(samplerate);


the Threshold widget via software to the default or hardware driver
value (DSlogic has already downloaded this value).

The misconduct arises now, since the value of the threshold widget is
zero, this is again downloaded after RUN command on the HW.
Comment 1 Jörg 2018-04-02 10:32:18 CEST
Created attachment 404 [details]
log-File without threshold
Comment 2 Jörg 2018-04-02 10:35:22 CEST
Created attachment 405 [details]
sr-File without threshold

uart_dump_ohne_threshold.*

uart speed 9600bd,
without threshold widget (0V),
phantom-trigger
Comment 3 Jörg 2018-04-02 10:36:41 CEST
Created attachment 406 [details]
pic-file without threshold
Comment 4 Jörg 2018-04-02 10:39:29 CEST
Created attachment 407 [details]
log-File with threshold + edge-trigger


uart_dump_mit_threshold_1_2_mit_flanketrigger_5log*
uart speed 9600bd,
using threshold widget (1.2V),
Trigger: edge-triggered
Comment 5 Jörg 2018-04-02 10:40:17 CEST
Created attachment 408 [details]
pic-file with threshold + trigger
Comment 6 Jörg 2018-04-08 18:13:26 CEST
(In reply to Soeren Apel from comment #0)
> As per mail from Joerg Alpers ("Pluseview Threshold every start Zerro
> (Dslogic)") on the ML, there appears to be a bug in PV. I quote:

> The misconduct arises now, since the value of the threshold widget is
> zero, this is again downloaded after RUN command on the HW.

My assumption that the set value zero is returned to the HW is not recognizable by the log files.
File comparison * ohne_threshold (1) * mit_threshold_1_2 (2)

yielded for (1) just an additional set command

sr: [00:01.956187] hwdriver: sr_config_set(): key 50001 (limit_samples) sdi 0x1a82c60 cg NULL -> uint64 1000000sr: 
[01:00.451363] hwdriver: sr_config_set(): key 30000 (samplerate) sdi 0x1a82c60 cg NULL -> uint64 500000


for (2) set command for threshold (due to misplaced widget)

sr: [00:01.899196] hwdriver: sr_config_set(): key 50001 (limit_samples) sdi 0x2495160 cg NULL -> uint64 1000000
sr: [00:07.734369] hwdriver: sr_config_set(): key 30000 (samplerate) sdi 0x2495160 cg NULL -> uint64 500000
sr: [00:17.803487] hwdriver: sr_config_set(): key 30023 (voltage_threshold) sdi 0x2495160 cg NULL -> (1.2, 1.2)
sr: [00:17.803680] hwdriver: sr_config_set(): key 50001 (limit_samples) sdi 0x2495160 cg NULL -> uint64 1000000
sr: [00:17.803946] hwdriver: sr_config_set(): key 30000 (samplerate) sdi 0x2495160 cg NULL -> uint64 500000


This explains:
the Dslogic driver has a bug if it is only used with the default threshold setting. The commands when transferring the default values ​​have no effect on threshold.
Reading back with the Get command probably shows the used value 1.3999 but triggering on this value does not happen !!!


I use Dslogic Pro !
Comment 7 Jörg 2018-04-08 18:25:30 CEST
I also noticed this behavior with sigrok-cli (see ML)
Comment 8 Uwe Hermann 2018-04-28 22:06:30 CEST
On the PulseView side this is likely to be at least an issue with floats/doubles. Currently the comparison seems to happen via a Glib::VariantBase equal() method, which checks the type and *exact* match of the value, AFAICS.

However, for floats and doubles this will rarely work correctly, since they can always differ due to the usual float/double properties.

Example in Enum::get_widget in pv/prop/enum.cpp:

  qDebug() << << variant.print().c_str() << v.first.print().c_str();

  (1.3999999999999999, 1.3999999999999999) (1.4000000000000001, 1.4000000000000001)

For float/double the check should be something like "fabs(a - b) < epsilon" instead.


More info e.g. here:

http://en.cppreference.com/w/cpp/types/numeric_limits/epsilon

https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/

I'll look into sigrok-cli and potential issues there.
Comment 9 Jörg 2018-04-29 09:50:53 CEST
The rounding problems have already been solved in Pulsview. After all, the display for setting the threshold threshold in the combo box for hardware properties x.x is valuable. It even creates a text with it.

Something is missing:
the current default value, which has now been set by the driver in the HW to read back through pulseview and set in the combo box through a routine.

See also my QT-Source-suggestion from the Maillinglist.
Comment 10 Uwe Hermann 2018-05-07 00:39:51 CEST
Fixed in a2f96263f56e019f811a7fc2eba3129ede1d9a5f, thanks!

As far as I've tested this issue seems fixed now. PulseView shows the correct voltage threshold upon startup and also when changing it via the UI dropdown. The values also seem to match what libsigrok is being told to set (or thinks the value is currently).

There's an additional issue with DSLogic triggering, #1188, but that's an independent issue and seems to be a pure driver issue.

Please re-test and let us know in case this specific issue is still causing problems. Thanks!
Comment 11 Jörg 2018-05-07 11:02:48 CEST
Created attachment 415 [details]
pic-file  threshold ComboBox
Comment 12 Jörg 2018-05-07 11:12:11 CEST
Testing Pulseview

Threshold Level Zerro !!

Error still exists  !!!

compile and use

jo@debian:~/sigrok_check_patch_threshold_pv_from_git_07_05_2018/bin$ SIGROK_FIRMWARE_DIR=/usr/local/share/sigrok-firmware/dslogic_firmware/v0_97 ./pulseview -V
PulseView 0.5.0-git-b10b58f


All values of the display and selection element are the 1st value of the element list!

Is the "Default Threshold" actively read back from the HW during the fix?
The list of possible threshold values does not contain the explicit information which is the default value - (here the selection value)!

If you do not use the DSLogic Pro or Plus for testing it might be that it does not stand out because the Basic only delivers two list values!
And thus also a trigger takes place, since both values work!
Comment 13 Jörg 2018-05-07 11:32:00 CEST
Please wait have clone wrong git pulseview 
My mistake. 

Sorry, test again !
Comment 14 Jörg 2018-05-07 11:56:26 CEST
Created attachment 416 [details]
BugFix OK  pic-file  threshold ComboBox

BugFix works.

Have enclosed picture.

thank you