-
Notifications
You must be signed in to change notification settings - Fork 156
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
Fix the exception when configure unsupported frequency #397
base: master
Are you sure you want to change the base?
Conversation
1. Add verify_config_laser_frequency to check frequency eariler 2. Catch the exception for set_laser_freq Signed-off-by: chiourung_huang <[email protected]>
@@ -1323,18 +1323,31 @@ def configure_tx_output_power(self, api, lport, tx_power): | |||
self.log_error("{} configured tx power {} > maximum power {} supported".format(lport, tx_power, max_p)) | |||
return api.set_tx_power(tx_power) | |||
|
|||
def configure_laser_frequency(self, api, lport, freq, grid=75): | |||
_, _, _, lowf, highf = api.get_supported_freq_config() | |||
def verify_config_laser_frequency(self, api, lport, freq, grid=75): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a description for this function?
Also, are we not planning to handle other frequency grid checks in this function?
# If user requested frequency is NOT the same as configured on the module | ||
# force datapath re-initialization | ||
if 0 != freq and freq != api.get_laser_config_freq(): | ||
if self.verify_config_laser_frequency(api, lport, freq) == True: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this to configure_laser_frequency?
Since with the current implementation, a port could remain uninitialized forever if the configured frequency is invalid.
Moving the check to configure_laser_frequency will allow to proceed with CMIS initialization with default frequency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 1571: need_update = self.is_cmis_application_update_required(api, appl, host_lanes_mask)
If the configured frquency is invalid, then it would not change the value of need_update.
If there is some other situation that requires initialization, it will still do the initialization.
If only the frequency is different and the frequency is invalid, I think it is not necessary to do initialize.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 1571: need_update = self.is_cmis_application_update_required(api, appl, host_lanes_mask)
If the configured frquency is invalid, then it would not change the value of need_update.
If there is some other situation that requires initialization, it will still do the initialization.
If only the frequency is different and the frequency is invalid, I think it is not necessary to do initialize.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please attach the Unit-test results with user configuring valid frequency and invalid frequency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chiourung could you address comment?
Description
Fix the issue #322
Motivation and Context
=> It doesn't need to re-init CMIS when configure unsupported frequency.
Original: re-init CMIS when configure unsupported frequency
After patch the implementation, it would skip to re-init if only invalid frequency is configured.
It still can do CMIS init, if CMIS application updated is required
Signed-off-by: chiourung_huang [email protected]