-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[Ufispace][PDDF] Add support for S9300-32D platform #14922
Conversation
@FuzailBrcm pls help review |
sync with latest master and reopen, please help to review |
try: | ||
# The index will start from 1 | ||
# sfputil already convert to physical port index according to config | ||
sfp = self._sfp_list[index] |
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 check this once again. Generally, the index passed down from 'xcvrd' to chassis.py is 1 based. So, to get the required sfp obj from _sfp_list, you need to do 'index-1'.
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 check this once again. Generally, the index passed down from 'xcvrd' to chassis.py is 1 based. So, to get the required sfp obj from _sfp_list, you need to do 'index-1'.
I take back my comment. I just checked your port_config.ini and that is using 0-based indexing. So no requirement of 'index-1'. However, this function is not required then as the parent class 'chassis_base.py' provide the same definition.
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.
Yes, you're right, the definition is duplicated, will update it.
status = True if (speed != 0) else False | ||
return status | ||
|
||
def get_target_speed(self): |
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.
target speed is the speed intended from the duty-cycle (or pwm) setting. I can understand of the PSU-fan intended speed is fixed but how can System fan target_speed is fixed at 65%?
Are you not supporting fan speed variations?
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.
Since the platform doesn't support pwm setting and the fan speed is controlled by ucd accroding to thermal sensor. Is that ok to set the target speed to crrent speed percentage to avoid warning ?
|
||
return direction | ||
|
||
def get_presence(self): |
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.
This implementation is pretty similar to the one present in pddf_fan.py. Can you double check why you had to overwrite that def with this one?
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.
Currently, the base always set the psu fan presence to true which is FRU in our platform. So we overwrite it to dectect PSU presence as for psu fan presence.
{ | ||
"capability": | ||
{ | ||
"ro": ["SYS_LED", "FAN_LED", "PSU1_LED", "PSU2_LED"], |
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.
This 'rw' or 'ro' flags are supported in pddf-device.json structure now. Please check this example below,
"LOC_LED":
{
"dev_info": { "device_type":"LED", "device_name":"LOC_LED"},
"dev_attr": { "index":"0", **"flag": "rw"**},
"i2c" :
{
"attr_list":
[
{"attr_name":"blue_blink", "descr": "" , "bits" : "5:0", "value" : "0x27", "swpld_addr" : "0x60", "swpld_addr_offset" : "0x25"},
You can make use of this provision.
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.
Thanks for your kindly notification, will update this
2. use flag to set led w/o in device config instead of validation in API. 3. update led color from amber to yellow 4. set fan target speed to current speed
@FuzailBrcm update commited and pass the check, pls help to review, thanks |
Changes look good to me. Approved. |
@lguohan FuzailBrcm has reviewed and approved, please help to merge, thanks |
@lguohan please help to merge, thanks |
Related work items: sonic-net#94, sonic-net#13789, sonic-net#14149, sonic-net#14515, sonic-net#14788, sonic-net#14922, sonic-net#14933, sonic-net#15284, sonic-net#15383, sonic-net#15464, sonic-net#15519, sonic-net#15521, sonic-net#15575, sonic-net#15636, sonic-net#15652, sonic-net#15684, sonic-net#15708, sonic-net#15725, sonic-net#15739, sonic-net#15755, sonic-net#15756, sonic-net#15757
Signed-off-by: linyutsung [email protected]
Why I did it
Add PDDF support on Ufispace S9300-32D with Broadcom ASIC
How I did it
Add PDDF configuration files, scripts and python files
How to verify it
Run pddf commands and show command
Which release branch to backport (provide reason below if selected)
Tested branch (Please provide the tested image version)
Description for the changelog
Link to config_db schema for YANG module changes
A picture of a cute animal (not mandatory but encouraged)