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

Add support for AZFP 130kHz #1412

Merged
merged 3 commits into from
Dec 9, 2024

Conversation

ctuguinay
Copy link
Collaborator

@ctuguinay ctuguinay commented Nov 19, 2024

As indicated by Steve Pearce from ASL, I used the same transmit frequency, pulse length, Sv compensation mapping that 125kHz has for 130kHz.

This addresses #1304, #1305.

@codecov-commenter
Copy link

codecov-commenter commented Nov 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.31%. Comparing base (9f56124) to head (982264d).
Report is 156 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1412      +/-   ##
==========================================
- Coverage   83.52%   80.31%   -3.21%     
==========================================
  Files          64       18      -46     
  Lines        5686     3089    -2597     
==========================================
- Hits         4749     2481    -2268     
+ Misses        937      608     -329     
Flag Coverage Δ
unittests 80.31% <ø> (-3.21%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@ctuguinay ctuguinay marked this pull request as ready for review November 19, 2024 06:46
@ctuguinay ctuguinay requested a review from leewujung November 19, 2024 06:46
@ctuguinay
Copy link
Collaborator Author

Oh wait, perhaps I don't need to check all 3 locations for that frequency haha. Let me change that in the morning.

@ctuguinay
Copy link
Collaborator Author

@leewujung This should be ready for review

leewujung
leewujung previously approved these changes Nov 20, 2024
Copy link
Member

@leewujung leewujung left a comment

Choose a reason for hiding this comment

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

Thanks @ctuguinay! This looks great.

The only comment I have is to add also a dict item for 120000.0 since ASL said that the same 150us and 250us mapping could be used for 120, 125, and 130 kHz transducers. Feel free to merge once that is added.

@ctuguinay
Copy link
Collaborator Author

@leewujung Oh huh apparently still needs you approval to merge something so small 😆

@leewujung leewujung self-requested a review December 9, 2024 18:01
@leewujung
Copy link
Member

Oh wow, ok. Sorry I had missed this - will merge now!

@leewujung leewujung merged commit f14e0c0 into OSOceanAcoustics:main Dec 9, 2024
5 checks passed
@leewujung leewujung added this to the v0.9.1 milestone Dec 9, 2024
@ctuguinay ctuguinay deleted the add_130kHz_azfp_support branch December 12, 2024 17:40
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