<d1b2>
<fredzo_72653> @josHua after sleeping on it I found a better way: I created an AcquisitionStopped() helper method on Osciolloscope class for the drivers to tell the download monitor that no more waveforms are expected and put the channels in DOWNLOAD_NONE state. This is much cleaner I think and easier to integrate in drivers.
<d1b2>
<josHua> let me have a look, I think that is probably good too
gruetzkopf has quit [Read error: Connection reset by peer]
gruetzkopf has joined #scopehal
<d1b2>
<josHua> @fredzo_72653 I don't understand this diff diff - if(stat == Oscilloscope::TRIGGER_MODE_STOP) + if(stat == Oscilloscope::TRIGGER_MODE_STOP || stat == Oscilloscope::TRIGGER_MODE_RUN || stat == Oscilloscope::TRIGGER_MODE_TRIGGERED) { // Final state triggerUpToDate = true;
<d1b2>
} what does this change?
<d1b2>
<josHua> actually I do not understand 1d6cd5b9fde8c7885d8e0b66c430dc95992a943d in general
<d1b2>
<fredzo_72653> This is to prevent looping on PollTrigger() while the scope is in Run mode but ngscopeclient is not in acquisition.
<d1b2>
<fredzo_72653> The other part I removed in this commit was here to circonvey the fact that channel downloads were no longer going to DOWNLOAD_NONE state after download, it's no longer needed.
<d1b2>
<azonenberg> The other question is how this is going to play with things like thunderscope
<d1b2>
<azonenberg> or picoscope
<d1b2>
<azonenberg> that report always-triggered and then just block on the read call
<d1b2>
<azonenberg> that is probably going to have to get refactored?
<d1b2>
<fredzo_72653> That should not ne a problem, the code above is only used when IsTriggerArmed() is false
<d1b2>
<azonenberg> i meant more like for progress display
<d1b2>
<azonenberg> we dont want to show 0% downloaded
<d1b2>
<fredzo_72653> Oh that
<d1b2>
<josHua> oh, hm, hang on, I think I have a way to make this work wit hthings still going to DOWNLOAD_NONE after downloaded. here hang on a second
<d1b2>
<azonenberg> so maybe we'd need to add a different callback to twinlan so that it doesn't report download in progress until the header is read
<d1b2>
<azonenberg> or actually maybe we can just do it in the driver?
<d1b2>
<azonenberg> dont report ready to download until we get the header with the timestamps etc
<d1b2>
<azonenberg> and then block while downloading waveform data?
<d1b2>
<fredzo_72653> Yeah someone with a thunderscope/picoscope will have to check that... but if they're fast enough and we never want to show a progress bar for them, the driver can always report DOWNLOAD_FINISHED and that should be good
<d1b2>
<azonenberg> (anyway i think all of those drivers are ones i maintain so i can take care of that post merge)
<d1b2>
<azonenberg> We still want the option to show a progress bar if using it over wifi, vpn, etc
<d1b2>
<josHua> here, hang on
<d1b2>
<azonenberg> just because it's fast plugged directly into it doesnt mean it'll be fast in all possible use cases
<d1b2>
<fredzo_72653> Yeah in that case the best way would be to implement progress calls in the transport layer like Joshua did on TCP and let the front decide if a progress bar should be shown.
<d1b2>
<azonenberg> Yeah
<d1b2>
<azonenberg> Twinlan is still TCP just with separate sockets for control and data plane
<d1b2>
<azonenberg> VICP is TCP with some custom framing on top
<d1b2>
<azonenberg> i'll likely add the callbacks to each after mereg
<d1b2>
<fredzo_72653> Yeah good idea too! I guess both work well, it's a matter of UI choice whether you prefer the fixed ACTIVE badge or the HDD-like one, I'll let you guys decide 😉
<d1b2>
<azonenberg> Preference setting
<d1b2>
<azonenberg> We always want an option to disable any kind of flashing indicator
<d1b2>
<azonenberg> If nothing else for a11y reasons, some people find it very distracting to have things flashing in peripheral vision etc
<d1b2>
<azonenberg> I dont think we have that written down as any kind of official UI guideline, but it should be treated as such
<d1b2>
<fredzo_72653> Works for me, @josHua I move the DOWLOAD_NONE state back to the end of AcquireData() where you put it in the first place, to be consistent with you last commit on UI side. We'll see later how to make the HDD-like display configurable.
<d1b2>
<azonenberg> Great i'll have a look when i'm free latr
<d1b2>
<fredzo_72653> @josHua I have a patch to make the speed detection global to an instrument rather the per channel. Is it OK for you if I push that ?
<d1b2>
<josHua> sure, that is fine, it is a little trickier that way to be consistent
<d1b2>
<josHua> but if it works it is fine
<d1b2>
<fredzo_72653> Tested and works fine on Siglent and Demo 👍
<d1b2>
<fredzo_72653> Don't hesitate to correct my English in that long comment ! 😅
<d1b2>
<josHua> my opinion has always been that developer-facing commentary 1) is allowed to be imperfect, and 2) is allowed to display some of the personality of the author 🙂
<d1b2>
<josHua> I always enjoy trying to figure out who wrote some code by figuring out whose voice the comment is written in
<d1b2>
<josHua> any time I have user-facing stuff I try to polish it up when I see it
<d1b2>
<josHua> I think I sign off on scopehal#913 and scopehal-apps#765 as at least being ready for review by @azonenberg
<d1b2>
<josHua> by this I mean 'I do not think you will be wasting your time by reading it at this stage'
<d1b2>
<azonenberg> Ok
<d1b2>
<josHua> I really need to make a pass on the diff to make it conform to the scopehal style. I think I have some Zofran left over from my shoulder surgery so I will take some in advance 😉
<d1b2>
<azonenberg> "a one based float value"
<d1b2>
<azonenberg> thats a little unclear
<d1b2>
<azonenberg> do you mean progress from 0 to 1?
<d1b2>
<fredzo_72653> Yep
<d1b2>
<azonenberg> Clarify that and i'm good to merge 913