Hello Brian,
I'd like to apologize for my late response, life does have that
nasty habit of getting in the way of fun activities :)
> I don't think my branch is the best possible way to do it, and I
> don't care if
> you don't do it exactly as I did it, but I think it is important that
> it be done.
> I am certain that others feel this way too.
I agree, and for me, the question is whether you're willing to
make it sound (is that a bad pun?) or whether you'd like to be
just done with it.
The things I'd like to see changed are:
1) The trigger settings are a property of the device, so they
should ideally be stored in pv::devices::HardwareDevice.
Since that would be a good chunk of work, letting the next
layer up the stack (Session) handle it is good, too.
2) The timer is part of the session state handling, so it should
be part of Session, not MainWindow.
3) The device config dialog is valid only for a single device,
so it should be accessible through the main view's toolbar.
This means the trigger setup dialog should come up when an
appropriate icon on pv::toolbars::MainBar is clicked.
4) As for the icon itself, using a trigger symbol (e.g.
trigger-marker-rising.svg or a variation thereof) would be
good in my opinion.
5) Session::report_failure() seems to not do anything meaningful,
so I'd like to see it replaced by a direct call to
capture_failure() instead.
Is that a bug fix, btw? I don't see the direct relation to the
rest of the commit since your code doesn't set an error.
Whether you make these changes or I do them depends on the
amount of motivation you have. I do agree that the feature
should be available either way.
> What is important is that it work for all logic analyzers, whether
> they provide
> internal support for it or not. Especially the FX2 class, which is
> what I usually
> use for this particularly undemanding task.
I agree completely here, too.
> My version operates by simulating clicking on the run button on the
> controls.
> That way it inherently can operate any logic analyzer. The biggest
> drawback
> to this approach is that after the rearm timer expires, the trace
> display goes
> away until there is a new trigger event, and that is undesirable. It
> would be
> better if the display remained visible until the new data is
> delivered from libsigrok.
> That may dictate the retrigger action be taken within libsigrok,
> which is OK with me.
Agreed. I suggest we treat each new acquisition like a new (set of)
frame(s). Currently, Session::sample_thread_proc() does this:
{
lock_guard<recursive_mutex> lock(data_mutex_);
cur_logic_segment_.reset();
cur_analog_segments_.clear();
}
highest_segment_id_ = -1;
frame_began_ = false;
If we did this only for the very first acquisiton (and not for any
repeated ones), this should work automagically. I haven't tried it,
though.
> Also, the way I overloaded the run/stop button to also enable/disable
> the repeat action
> is rather opaque and could probably be improved by changing the
> run/stop button
> to a run/stop/rearm button, or perhaps two separate buttons. I am
> not overly
> proud of how I did it, I just wanted to minimize changes for the
> branch.
That's fine, the "waiting for rearm timeout" state can be added by
a later change anyway.
> abraxa, I recall you wanted to redo this area anyway, so I invite you
> to do so.
> I would be willing to help work on it, but I don't know where you
> want to take it.
Now you know :)
Thanks for contributing, Brian!
All the best,
-Soeren
>
> I will drop by IRC tomorrow if you want to talk about it. I left
> this on the mailing
> list because I think it is too long for an IRC message, and wanted it
> more persistent.
>
> My repeat branch is at:
>
> https://github.com/Cenkron/pulseview branch repeat
>
> I have already rebased it to the most recent pulseview master.
>
> Brian Johnson, aka Cenkron
>
> -------------------------------------------------------------------
> -----------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> sigrok-devel mailing list
> sigrok-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/sigrok-devel
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
sigrok-devel mailing list
sigrok-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sigrok-devel
