Platform Driver Development Framework (PDDF): 1) Changes to psu base class (1.0 APIs) to support PDDF CLI utils, 2) Adding fan_base class to support PDDF fan CLI utils (comments incorporated) #62
Conversation
…class (1.0 APIs) to support PDDF CLI utils, 2) Adding fan_base class to support PDDF fan CLI utils (comments incorporated)
sonic_fan/fan_base.py
Outdated
| def get_speed_rear(self, index): | ||
| """ | ||
| Retrieves the speed of a rear FAN in the tray in revolutions per minute defined | ||
| by 1-based index <index> |
There was a problem hiding this comment.
my understanding is that although there could have multi fans(usually 2) in a tray, but each fan has its own tachometer, so these fans in the same tray should still be treated as separated fans, In my view, this function is not needed.
There was a problem hiding this comment.
While mentioning the platform inventory, one fan-tray is usually considered as one fan unit. ODM too, provide the APIs to retrieve 'rear-fan' speed. 'lm-sensors' (or sensors command) denotes rear fan of FAN tray 1 as fan11, rear fan of FAN tray 2 as fan22 etc.
Open Config model FAN related CLIs also provide the fan speed in term of front and rear speed.
Set speed FAN APIs also set speed of both the fans in a single call. Hence a separate API is provided for rear FAN speed.
There was a problem hiding this comment.
on some vendors platform, they do treat fans in the same tray as separated fans and hide the rear/front info from user, in this case, this function is not applicable.
There was a problem hiding this comment.
Hi Keboliu,
If some vendors treat 'rear' fans as separate, indexing them is left to discretion of the vendors. e.g Accton uses,
fan1 (front) --------- fan11 (rear)
fan2 (front) --------- fan12(rear) etc
Some other platform may use fan1 (tray1-front), fan2(tray1-rear), fan3 (tray2-front), fan4 (tray2-rear) etc.
If separate indexing is being used, vendors can use 'get_speed' API. If tray is numbered and fans inside a tray are mentioned as 'front' and 'rear' then let 'get_speed_rear' API be used. Since some vendors also use tray numbering, can't we keep both APIs?
There was a problem hiding this comment.
I am ok with keeping both APIs.
There was a problem hiding this comment.
I think this function may require more thorough commenting to ensure vendors understand that get_speed_rear() is only to be implemented for the rear fan of a 2-fan tray.
There was a problem hiding this comment.
Added extra comment to mention the same.
sonic_psu/psu_base.py
Outdated
| """ | ||
| return 0 | ||
|
|
||
| def get_fan_rpm(self, idx, fan_idx): |
There was a problem hiding this comment.
Since fan speed unit is RPM, this name is chosen for the API. I can change this to 'get_fan_speed' if you think that represents the objective better.
There was a problem hiding this comment.
if you look at the function names in fan_base, they are using 'speed', I would suggest using 'speed' for consistency.
There was a problem hiding this comment.
Sure. I will update the PR
| Retrieves the output power in micro watts of a power supply unit (PSU) defined | ||
| by 1-based index <idx> | ||
| :param idx: An integer, 1-based index of the PSU of which to query o/p power | ||
| :return: An integer, value of o/p power in micro Watts if PSU is good, else zero |
There was a problem hiding this comment.
the return value should be a float? same comments for the above voltage, current get functions.
There was a problem hiding this comment.
Correct. However, psu driver takes care of converting the float value into milli or micro unit depending upon the required granularity. The SysFS interface reads the converted values.
There was a problem hiding this comment.
what's your decision on return values? Still in integer or change to float?
There was a problem hiding this comment.
We have decided to go with integer values here with milli volts, amps etc granularity.
| """ | ||
| return "" | ||
|
|
||
| def get_mfr_id(self, idx): |
There was a problem hiding this comment.
can you give an example string for this id? Not sure the difference between mfr_id and serial number.
There was a problem hiding this comment.
mfr_id is the 'Manufacturer's ID'. Either name or some abbreviation which denotes the manufacturer. e.g.
- On Delta platforms, PSU mfr_id is 'DELTA'
- On some Accton platforms, PSU mfr_id is '3Y POWER'
- On soem Quanta platforms, PSU mfr_id is 'ARTESYN'
|
Changes as per the feedback --- 824881f Above is the change incorporating the feedbacks from keboliu. |
|
@fk410167 Added extra comment explaining the usage of get_speed_rear() |
|
Thanks keboliu for approving. sonic-buildimage PR ( sonic-net/sonic-buildimage#3387 ) has dependency over this PR ( #62 ) and sonic-utilities PR ( sonic-net/sonic-utilities#624 ). |
sonic-net/sonic-platform-common#71 [Bug fix] fix a syntax error which was introduced by last commit sonic-net/sonic-platform-common#70 Fix EEPROM vendor extension TLV number counting issue sonic-net/sonic-platform-common#62 Platform Driver Development Framework (PDDF): 1) Changes to psu base class (1.0 APIs) to support PDDF CLI utils, 2) Adding fan_base class to support PDDF fan CLI utils (comments incorporated) sonic-net/sonic-platform-common#68 DeviceBase.get_name should raise NotImplementedError like other member
All these changes are w.r.t 1.0 APIs. These will be migrated to 2.0 APIs in future. Comments to
#57
are addressed in this PR.