Bug 1794 - The checkbox "Show time at the trigger" does not work
Summary: The checkbox "Show time at the trigger" does not work
Status: CONFIRMED
Alias: None
Product: PulseView
Classification: Unclassified
Component: Other (show other bugs)
Version: unreleased development snapshot
Hardware: All All
: Normal minor
Target Milestone: ---
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-08-22 18:31 CEST by Brian
Modified: 2022-09-01 20:39 CEST (History)
1 user (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Brian 2022-08-22 18:31:10 CEST
The checkbox "Show time at the trigger" does not work.
Checked or unchecked, it does nothing.

I have diagnosed the problem, in the file pv/views/trace/view.cpp
Two "fixes" made it work.

1. When applying the time offset the time at trigger needed to be subtracted from (rather than added to) the signal time.  This was just a typical coding error, and my "fix" corrects that.

2. There are two functions (methods) provided for managing the value of a display time offset:

void View::set_zero_position(const pv::util::Timestamp& position)
void View::reset_zero_position()

These two functions support multiple different activities:

- session initialization
- to restore settings during session reload.
- to support a graphically applied zero time point.
- to support the "Show time..." checkbox.
- when updating the capture state

These different purposes require different algorithms, sharing only the need to adjust the time offset.

As a result, the semantics of these two functions are somewhat confused, as the algorithm attempts to determine what to do for each case based on the program state.

It should be remembered that the "Show time..." checkbox only has a substantive effect when the pre-trigger capture ratio setting is greater than zero, because only then is the trigger time not zero.

So, my second "fix" was to further complicate the semantics, to "correct" the algorithm for the "Show time..." checkbox requirements.  This is not a clean fix, but seems to work for the "Show time..." case.  I don't know for certain about all the other cases, because it is too complex.

Rather than adopt my second "fix", both of these functions should be rewritten to deconfuse them.

My fixes can be obtained from:
  https://github.com/Cenkron/pulseview -b fix-trigger-zero.
Comment 1 Brian 2022-08-23 17:57:33 CEST
Having been unable to not think about this problem, here is my suggestion for how to restructure this.

1. Separate the code handling each of the "reasons" for manipulating the variable zero_offset.

2. Remember the intended zero_offset value separately for each reason.

   "Reasons" may/should include the following (and maybe others)
   1. Change of session, both automatically and via the UI
   2. Change of capture state, both automatically and via the UI
   3. Change of "Show time..." checkbox via the UI
   4. Change of user set zero time marker via the UI

3. Provide a "validity" flag for each reason.  This already exists for the marker.
   (custom_zero_offset_set_)

4. Provide a new function to choose which value to use for setting zero_offset based on priority and validity. 
   Call this function whenever the validity of any of the "reasons" changes state.

5. Remembering them separately is important because the user can dynamically switch between them via the UI, even after capture ends.
Comment 2 Brian 2022-08-23 18:44:31 CEST
From the preceding comment, change the following sentence:

   Call this function whenever the validity OR VALUE of any of the "reasons" changes state.
Comment 3 Brian 2022-08-26 18:21:25 CEST
Further investigation revealed that when "Set time..." is working, then there is a problem with the save and restore mechanism.  It assumes that an offset is because of a manual time zero insertion, and so it cannot be turned on and off in the restored session.  I think this is also at the heart of the current bug.

I think that both zero offsets should be saved and restored, along with the checkbox settings, so that a restored session should work just like the original version.
Comment 4 Brian 2022-09-01 20:39:26 CEST
After completion of a fuller diagnosis, this is intended to replace all of the foregoing.

Bug 1794
The functionality underlying the checkbox "Show time at the trigger" is broken.
Checked or unchecked, it does nothing.

I have diagnosed the problem, most of which lies within the file pv/views/trace/view.cpp

Two functions (methods) are provided to support ruler class functionality provided for managing a display time offset:

void View::set_zero_position(const pv::util::Timestamp& position)
void View::reset_zero_position()

These two functions are normally called as a result of right clicking on the ruler bar and selecting set or clear of a custom time zero.

Somewhere along the line, the set function came to be (mis)used for another purpose.
In particular, a call to set a custom time zero at time zero, that did not, and could not, originate from the click on rler bar mechanism.
This resulted in a permanently set custom time setting on top of the actual time zero.
This resulted in the side effect of disabling the trigger time zero setting, because it is lower priority than a custom setting.
I cll it permanent because of a bug in the ruler class in which it is assumed that if the zero time is the default, then there cannot be a custom setting.
This results in the option to dismiss the custom setting being missing.
It is not quite permanent because this can all be circumvented by setting and cancelling a genuine (non-zero) custim time zero.
The view state was normally left in a hard to reset state of having a custom zero set when that was not actually the case.  

Two "fixes" withing view.cpp made the trigger-zero mechanism work.
These "fixes" can be found in https://github.com/Cenkron/pulseview -b diagnose-trigger-zero

1. When applying the time offset the time at trigger needed to be subtracted (rather than added) from the signal time.  This was just a typical coding error.

2. Making it impossible to set custom zero time on top of the real time zero.

Rather than adopt my "fix", the underlying problems should be addressed.
I have done that in https://github.com/Cenkron/pulseview -b dev-trigger-zero

The fixes in that set of three files do the following:

1. Fixes the bug in the custom zero position reset code in ruler.cpp

2. Fixes the bug in the bogus call to set a zero point on top of zero.

3. Allows the "set time..." checkbox to be effective even after the capture completes.