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 support for 25R1 EMI Receiver component RBW and RBW Factor feat… #5640

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

Conversation

cmesibov
Copy link
Contributor

@cmesibov cmesibov commented Jan 8, 2025

Description

This PR adds support for 25R1 EMI Receiver component RBW and RBW Factor settings.

Issue linked

Issue #5585

Checklist

  • I have tested my changes locally.
  • I have added necessary documentation or updated existing documentation.
  • I have followed the coding style guidelines of this project.
  • I have added appropriate tests (unit, integration, system).
  • I have reviewed my changes before submitting this pull request.
  • I have linked the issue or issues that are solved by the PR if any.
  • I have agreed with the Contributor License Agreement (CLA).

@ansys-reviewer-bot
Copy link
Contributor

Thanks for opening a Pull Request. If you want to perform a review write a comment saying:

@ansys-reviewer-bot review

@github-actions github-actions bot added the enhancement New features or code improvements label Jan 8, 2025
Copy link

codecov bot commented Jan 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.27%. Comparing base (b47ea37) to head (e1ec0a5).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5640      +/-   ##
==========================================
+ Coverage   85.25%   85.27%   +0.02%     
==========================================
  Files         152      152              
  Lines       60937    60951      +14     
==========================================
+ Hits        51952    51979      +27     
+ Misses       8985     8972      -13     

Copy link
Collaborator

@SMoraisAnsys SMoraisAnsys left a comment

Choose a reason for hiding this comment

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

Hi @cmesibov thanks for the contribution. There are a few things that should be modified in your contribution. First, could you add a description to your PR ?

Now more dev oriented:

  • Since you added attributes rbw and rbw_factor with default values, it doesn't make sense to create the property getters (or the behavior might not be the one you expect).
  • The previous default value for RBW was 9000Hz, could you leave that same default value for backward compatibility ?
  • From the previous behavior, I imagine that RBW must be a string so I'm not sure it's needed to cast it into a string in emission
  • Can you add some tests to ensure that things work as expected ?

Finally, you told me that you would test your changes on previous AEDT versions. Did you do that ?

@cmesibov
Copy link
Contributor Author

cmesibov commented Jan 9, 2025

@SMoraisAnsys , Thanks for the review. The RBW and RBW Factor settings are set by index, not by value. Therefore the text setting of "900MHz" was in error to begin with. The new init defaults of zero should be the default setting. I have tested the changes on my own project for 24R2 and 25R1. Would adding RBW and RWB Factor settings to test_76_emi_receiver be adequate?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features or code improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants