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

portconfig option to configure Tx power and laser frequency of ZR transceiver module #2197

Merged
merged 6 commits into from
Jul 19, 2022

Conversation

prgeor
Copy link
Contributor

@prgeor prgeor commented Jun 7, 2022

What I did

Added new configuration CLI options to configure Tx power and laser frequency of ZR transceiver module

New command output (if the output of a command-line utility has changed)

root@sonic:~# portconfig -p Ethernet0 -P -28.3
Setting target Tx output power to -28.3 dBm on port Ethernet0
root@sonic:~# redis-cli -n 4 hgetall "PORT|Ethernet0" 
 1) "admin_status"
 2) "up"
 3) "alias"
 4) "Ethernet1/1"
 5) "index"
 6) "1"
 7) "lanes"
 8) "1,2,3,4,5,6,7,8"
 9) "laser_freq"
10) "1000"
11) "speed"
12) "400000"
13) "tx_power"
14) "-28.3"
root@sonic:~# 
root@sonic:~# portconfig -p Ethernet0 -F 191000
Setting laser frequency to 191000 GHz on port Ethernet0
root@sonic:~# redis-cli -n 4 hgetall "PORT|Ethernet0" 
 1) "admin_status"
 2) "up"
 3) "alias"
 4) "Ethernet1/1"
 5) "index"
 6) "1"
 7) "lanes"
 8) "1,2,3,4,5,6,7,8"
 9) "laser_freq"
10) "191000"
11) "speed"
12) "400000"
13) "tx_power"
14) "-28.3"
root@sonic:~# 

@prgeor prgeor requested a review from qiluo-msft June 11, 2022 00:20
@prgeor
Copy link
Contributor Author

prgeor commented Jun 11, 2022

@qiluo-msft can you please review?

sys.exit(1)
port.set_tx_power(args.port, args.tx_power)
if args.laser_freq:
if args.laser_freq < 1:
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 15, 2022

Choose a reason for hiding this comment

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

args.laser_freq < 1

args.laser_freq <= 0 is more readable. #Closed

@@ -307,6 +326,17 @@ def main():
port.set_adv_interface_types(args.port, args.adv_interface_types)
if args.tpid:
port.set_tpid(args.port, args.tpid)
if args.tx_power:
d = decimal.Decimal(str(args.tx_power))
if d.as_tuple().exponent < -1:
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 15, 2022

Choose a reason for hiding this comment

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

d.as_tuple().exponent < -1

Could you add testcase to cover this branch? #Closed

sys.exit(1)
port.set_tx_power(args.port, args.tx_power)
if args.laser_freq:
if args.laser_freq <= 0:
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 15, 2022

Choose a reason for hiding this comment

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

args.laser_freq <= 0:

Could you add testcase to cover this branch? #Closed

result = self.basic_check("frequency", ["Ethernet0", "191300"], ctx)
assert "Setting laser frequency" in result.output
result = self.basic_check("frequency", ["Invalid", "191300"], ctx, op=operator.ne)
result = self.basic_check("frequency", ["Ethernet0", "-191300"], ctx, op=operator.ne)
Copy link
Contributor

Choose a reason for hiding this comment

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

result

Are you testing a failure case? If yes, validate the result?

result = self.basic_check("tx_power", ["Ethernet0", "27.3"], ctx)
assert "Setting target Tx output power" in result.output
result = self.basic_check("tx_power", ["Invalid", "27.3"], ctx, op=operator.ne)
result = self.basic_check("tx_power", ["Ethernet0", "27"], ctx)
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 15, 2022

Choose a reason for hiding this comment

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

result

Are you testing a failure case? If yes, validate the result? #Closed

#self.basic_check("link-training", ["Ethernet0", "on"], ctx)
result = self.basic_check("frequency", ["Ethernet0", "191300"], ctx)
assert "Setting laser frequency" in result.output
result = self.basic_check("frequency", ["Invalid", "191300"], ctx, op=operator.ne)
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 15, 2022

Choose a reason for hiding this comment

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

result

Better to validate each result with your expectation. #Closed

@yxieca
Copy link
Contributor

yxieca commented Aug 17, 2022

@prgeor this PR cannot be cherry-picked cleanly, can you create separate PR for 202205?

@abdosi
Copy link
Contributor

abdosi commented Aug 23, 2022

@prgeor can you create PR for 202205 ?

@prgeor prgeor deleted the ZR-config branch August 23, 2022 23:20
prgeor added a commit to prgeor/sonic-utilities that referenced this pull request Aug 23, 2022
…nsceiver module (sonic-net#2197)

* New CLI option for configuring Xcvr frequency and Tx power

* portconfig changes to configure xcvr's frequency and laser power

* Added unit tests

* Added check for tx_power and freq

* Address review comment

* Address review comment
liat-grozovik pushed a commit that referenced this pull request Aug 25, 2022
…nsceiver module (#2197) (#2330)

Cherry-pick of PR #2197

Added new configuration CLI options to configure Tx power and laser frequency of ZR transceiver module
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.

4 participants