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

Added Frequency vs Throttle Spectrum Graph #322

Merged
merged 1 commit into from May 4, 2019

Conversation

McGiverGim
Copy link
Member

This PR continues the serie of PR about the Spectrum Graph that we let here: #316

This PR adds the possibility to show the Frequency vs Throttle in the Spectrum graph:
image

The sliders in the maximized version move the frequency and the intensity of the heat scale.

I cached all that I could but some things are slow. I will try to optimize it in future PR if I find a way.

My math skills are very limited, so thanks to @bw1129 the author of PidToolBox for point me in the right direction.

I'm pretty sure there are things that can be done better, I imitated the way it was done in the standard frequency spectrum, so if someone with better knowledge of maths want to take a look, there are some things that I'm not sure why are done in this way, specially related with the size of the arrays passed to the FFT.
I think that the future PR can be:

  • Refactor the spectrum_graph.js file and divide it into two, one for control and the other for the calcs.
  • Add the filters to the graph, like in the standard frequency graph.

@McGiverGim McGiverGim added this to the 3.4.0 milestone May 1, 2019
Copy link
Member

@mikeller mikeller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work, the graph looks hot!

ANALYSER_LARGE_LEFT_MARGIN = 10,
ANALYSER_LARGE_TOP_MARGIN = 10,
ANALYSER_LARGE_HEIGHT_MARGIN = 20,
ANALYSER_LARGE_WIDTH_MARGIN = 20;
ANALYSER_LARGE_WIDTH_MARGIN = 20,
FIELD_THROTTLE_NAME ='rcCommands[3]',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spacing. 👾

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

_fftData : null,
_mouseFrequency : null,
_sysConfig : null,
_isFullScreen : false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good example of why I am generally not in favour of aligning code - reviewing this is made harder by the fact that it had to be realigned...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I try to not do it, I understood the reasons the latest time, but I do it without thinking (at my real job we don't make revisions). 😁

mikeller
mikeller previously approved these changes May 1, 2019
mikeller
mikeller previously approved these changes May 1, 2019
@spatzengr
Copy link

spatzengr commented May 3, 2019

Left of BBE 3.3.0 and right is with the above PR. Spectrum of Debug values is showing different.

image

Throttle based waterfall seems fine though. Look like spectrum is mirroring plot.

image

@McGiverGim
Copy link
Member Author

Thanks for testing it @spatzengr !! I saw something similar yesterday, but it seemed to me that the old version shown it too. Is clear that not. I will try to fix it.

@McGiverGim
Copy link
Member Author

The problem was not with this PR. It was produced in other before. I have pushed a PR to fix it, but I'm not sure if it will work in all cases. Do not merge this until the other PR is merged, and I will see if this needs some changes.

@spatzengr if you can test the other PR (#323) to see if all is ok with this, with different blackbox files with different blackbox rates it will be great.
I'm not sure if the changes can modify the noise in the graph (move the maximum frequency noise, for example) respect to the version 3.3.0 that we suppose is ok.

@McGiverGim
Copy link
Member Author

@mikeller rebased against master. Ready to be merged ;)

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

Successfully merging this pull request may close these issues.

None yet

3 participants