Skip to content
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

Created new logic math signal #42

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KyleJ61782
Copy link
Contributor

Added a logic math signal so that logic signals can be combined using an
expression. This can be particularly useful when wanting to visually
identify interesting locations to investigate within a series of logic
traces.

This was accomplished by refactoring the math signal into mostly generic
code with descendants that flesh out the analog and logic specific code.

Added a logic math signal so that logic signals can be combined using an
expression. This can be particularly useful when wanting to visually
identify interesting locations to investigate within a series of logic
traces.

This was accomplished by refactoring the math signal into mostly generic
code with descendants that flesh out the analog and logic specific code.
@abraxa
Copy link
Member

abraxa commented Jun 22, 2021

Hello. I appreciate that you spent time on this but unfortunately, this is not merge-able. I really wish you had contacted me first to discuss what you're going to do.

For example, the underlying math library exprtk supports boolean operators, so it makes sense to add support for logic signals in the math signal. Sure, the math signal generates an analog signal but that can easily be converted back to a logic signal using a conversion. If that's too inconvenient, an automatic conversion than be added.
With that, I don't see the need for separate classes.

Also, you're violating the coding style by putting code in header files and you're placing the signal colors in files where they don't belong. data/analog.cpp and data/logic.cpp are merely holding data and are separate from signals.

Again, I wish you had spoken to me beforehand so that we could've aligned instead of ending up in this unfortunate situation.

@KyleJ61782
Copy link
Contributor Author

No worries! I provided the code in case it's useful. I'd certainly be open to reimplementing according to any and all suggestions you have. I agree that it's not the cleanest solution, for sure.

It's been a while, but I seem to recall one of the difficulties causing me to implement two separate classes was the fact that analog and logic signals have completely different storage underpinnings. However, any and all suggestions for proper implementation are very much so of interest.

@depili
Copy link
Contributor

depili commented Nov 20, 2022

Sometimes it would be useful to be able to for example invert a logic signal before passing it to another existing decoder. I don't know if math signals would allow such a thing generally, as I only have digital signals to work with. If the current implementation can do such things it would be nice to have an example.

Another use-case would be dealing with differential digital signals and trying to filter out noise.

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.

3 participants