Skip to content

Fix sample index and label handling #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

evan-a-a
Copy link
Contributor

@evan-a-a evan-a-a commented Feb 9, 2024

GetSampleNumber returns 0 for the first sample, so we don't need to subtract one when using AdvanceToAbsPosition. Doing so on recent versions of Logic results in the analyzer completely failing to analyze the waveform since subtracting 1 from a 0 value U64 results in the U64 equaling UINT64_MAX

GetSampleNumber returns 0 for the first sample, so we
don't need to subtract one when using AdvanceToAbsPosition.
Doing so on recent versions of Logic results in the analyzer
completely failing to analyze the waveform since subtracting 1
from a 0 value U64 results in the U64 equaling UINT64_MAX
@evan-a-a evan-a-a marked this pull request as draft February 10, 2024 21:49
@evan-a-a
Copy link
Contributor Author

I found a few other related issues that I'm going to add fixes for in this PR.

The end of the transaction is always the same sample as clock
for the cycle that is being parsed.
* Ensure strings are null initialized and terminated
* Remove data bit accounting for waveform view
* Fix corruption of dummy label on waveform view
@evan-a-a evan-a-a changed the title Fix sample index handling Fix sample index and label handling Feb 12, 2024
@evan-a-a evan-a-a marked this pull request as ready for review February 12, 2024 16:39
@AddioElectronics
Copy link
Owner

Thanks for the contribution, I will check and merge them tomorrow.

@AddioElectronics
Copy link
Owner

AddioElectronics commented Feb 28, 2024

Not subtracting 1 causes an error in the simulation, tested in Logic 2, KingstVIS, and newest Logic 2 Update.

The initial (idle) state of the CLK line does not match the settings.

It has been a while since I started the project, so my memory may be off, but its definitely there for a reason. I believe AdvanceToNextEdge is always called the first iteration, and I can confirm mCurrentSample is not 0 the first time its executed, at least for the clock/polarity config I was using. So I'm not sure how it was causing the analyzer to fail.

Can you tell me what the analyzer settings you were using? And were you using the simulator, or device when you experience the crashes?

I'm not sure why but I can't get logic 1 to attach a debugger right now, and logic 2 isn't hitting any breakpoints, I remember I had trouble with it before but don't really want to go through it all again.
If you can't get it working either, you can easily debug with the KingstVIS simulator.

So for now I think the best thing to do is just do a check for zero.
I'll merge your other changes and add the check.

AddioElectronics added a commit that referenced this pull request Feb 28, 2024
Apparently -1 was causing issues for a user in newer versions of logic.
AddioElectronics pushed a commit that referenced this pull request Feb 28, 2024
AddioElectronics added a commit that referenced this pull request Feb 28, 2024
Highlighting an issue raised in PR #2 commit e016e5f
@AddioElectronics
Copy link
Owner

e016e5f causes problems with spacing.
analyzer_spacing

Your other commit ce4ebfc has been merged.

I'll leave it open for a while if you want to give any input.

@evan-a-a
Copy link
Contributor Author

evan-a-a commented Feb 28, 2024

Can you tell me what the analyzer settings you were using? And were you using the simulator, or device when you experience the crashes?

I am capturing from a real device, with the following settings. Note: In my local modifications I have changed "Dummy Clocks" to "Dummy Cycles" to make configuration a bit easier.

Screenshot from 2024-02-28 13-21-55

I'm not sure why but I can't get logic 1 to attach a debugger right now, and logic 2 isn't hitting any breakpoints, I remember I had trouble with it before but don't really want to go through it all again.

I was able to debug in Logic 2, and from what I remember mCurrentSample starts at zero with the settings I am using.

e016e5f causes problems with spacing.

Here's what it looks like on my modified version with real data in Logic 2. Everything is aligned and analyzed as expected. Perhaps this is a difference between KingVTS and Logic 2?

Screenshot from 2024-02-28 13-23-36

@AddioElectronics
Copy link
Owner

I do like Dummy cycles more, its definitely more appropriate.

Thanks for posting your settings, I'll try them out tomorrow.

Ah ok I see what you mean now, I had it lining up to the start of the next clock cycle to give more room for the labels when zooming out, or else you just see "D".
But I see how it could be an issue when it stays low for long periods of time.
I think the best option is to add a checkbox or dropdown that allows you to select between the 2.

@evan-a-a
Copy link
Contributor Author

I do like Dummy cycles more, its definitely more appropriate.

I am planning to submit this as a PR, just fell down the priority list.

Ah ok I see what you mean now, I had it lining up to the start of the next clock cycle to give more room for the labels when zooming out, or else you just see "D". I think the best option is to add a checkbox or dropdown that allows you to select between the 2.

What about just dropping the D/Data labels entirely? It should be obvious from context that this is data.

@AddioElectronics
Copy link
Owner

AddioElectronics commented Mar 1, 2024

Sure that sounds good to me, I've already got the project open so I made the quick change.
Commit e016e5f is also merged.

@AddioElectronics
Copy link
Owner

Were you still going to make another pull request?
Just need to know if I should change it to Dummy Cycles or if you had that covered?
I had only removed the D/Data labels to merge that commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants