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.

Sign up for GitHub

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

Jump to bottom

Platform Driver Developement Framework (PDDF) #4756

Merged
merged 9 commits into from
Nov 12, 2020
Merged

Platform Driver Developement Framework (PDDF) #4756

merged 9 commits into from
Nov 12, 2020

Conversation

FuzailBrcm
Copy link
Contributor

@FuzailBrcm FuzailBrcm commented Jun 11, 2020 •
edited
Loading

Signed-off-by: Fuzail Khan fuzail.khan@broadcom.com

This change introduces PDDF which is described here,
sonic-net/SONiC#536

Most of the platform bring up effort goes in developing the platform device drivers, SONiC platform APIs and validating them. Typically each platform vendor writes their own drivers and platform APIs which is very tailor made to that platform. This involves writing code, building, installing it on the target platform devices and testing. Many of the details of the platform are hard coded into these drivers, from the HW spec. They go through this cycle repetitively till everything works fine, and is validated before upstreaming the code.
PDDF aims to make this platform driver and platform APIs development process much simpler by providing a data driven development framework. This is enabled by:

JSON descriptor files for platform data
Generic data-driven drivers for various devices
Generic SONiC platform APIs
Vendor specific extensions for customisation and extensibility

- Why I did it
Rapid development and qualification of different platforms on SONiC OS.

- How I did it
PDDF generic platform drivers and utils are provided under
sonic-buildimage/platform/pddf/i2c/

PDDF generic user space 2.0 platform APIs are added here,
sonic-buildimage/platform/pddf/platform-api-pddf-base/

Rest of the changes are added to enable the framework.

- How to verify it
We will add support for a sample platform to enable verification.

Under PDDF mode, pddf utils can be verified,

sudo pddf_psuutil

sudo pddf_fanutil

sudo pddf_thermalutil

sudo pddf_ledutil

- Description for the changelog

Platform Driver Development Framework (PDDF): Generic kernel device drivers and platform APIs

- A picture of a cute animal (not mandatory but encouraged)

Depends on:
sonic-net/sonic-platform-common#92
sonic-net/sonic-utilities#940

@lgtm-com
Copy link

lgtm-com bot commented Jun 11, 2020

This pull request introduces 121 alerts when merging be6ee68898e187413de3e9790d93be2e459b1556 into 4da4955 - view on LGTM.com

new alerts:

  • 54 for Unused local variable
  • 52 for Unused import
  • 7 for Variable defined multiple times
  • 4 for Wrong number of arguments in a class instantiation
  • 2 for Except block handles 'BaseException'
  • 2 for Syntax error

jleveque
jleveque suggested changes Jul 7, 2020
View reviewed changes
Copy link
Contributor

@jleveque jleveque left a comment

Choose a reason for hiding this comment

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

There is a large number of LGTM alerts. Please address.

@lgtm-com
Copy link

lgtm-com bot commented Aug 8, 2020

This pull request introduces 121 alerts when merging 3baa2df527df380ed045cdee00ef56cb2d64455e into 567da44 - view on LGTM.com

new alerts:

  • 54 for Unused local variable
  • 52 for Unused import
  • 7 for Variable defined multiple times
  • 4 for Wrong number of arguments in a class instantiation
  • 2 for Except block handles 'BaseException'
  • 2 for Syntax error

@lgtm-com
Copy link

lgtm-com bot commented Aug 9, 2020

This pull request introduces 22 alerts when merging 77e8fdec538d19b2aea7312df1a7d27de0d9ec1e into 78baece - view on LGTM.com

new alerts:

  • 9 for Unused local variable
  • 7 for Wrong number of arguments in a class instantiation
  • 5 for Unused import
  • 1 for Syntax error

@lgtm-com
Copy link

lgtm-com bot commented Aug 9, 2020

This pull request introduces 8 alerts when merging 26d735756c31a15493173731d49fcf6c91718420 into 78baece - view on LGTM.com

new alerts:

  • 7 for Wrong number of arguments in a class instantiation
  • 1 for Syntax error

FuzailBrcm
FuzailBrcm commented Aug 10, 2020
View reviewed changes
platform/pddf/platform-api-pddf-base/sonic_platform_pddf_base/pddf_chassis.py
self.platform_inventory = self.pddf_obj.get_platform()

# Initialize EEPROM
self.sys_eeprom = Eeprom(self.pddf_obj, self.plugin_data)
Copy link
Contributor Author

@FuzailBrcm FuzailBrcm Aug 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

lgtm [py/call/wrong-number-class-arguments]

FuzailBrcm
FuzailBrcm commented Aug 10, 2020
View reviewed changes
platform/pddf/platform-api-pddf-base/sonic_platform_pddf_base/pddf_chassis.py
# FANs
for i in range(self.platform_inventory['num_fantrays']):
for j in range(self.platform_inventory['num_fans_pertray']):
fan = Fan(i, j, self.pddf_obj, self.plugin_data)
Copy link
Contributor Author

@FuzailBrcm FuzailBrcm Aug 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

lgtm [py/call/wrong-number-class-arguments]

FuzailBrcm
FuzailBrcm commented Aug 10, 2020
View reviewed changes
platform/pddf/platform-api-pddf-base/sonic_platform_pddf_base/pddf_chassis.py

# PSUs
for i in range(self.platform_inventory['num_psus']):
psu = Psu(i, self.pddf_obj, self.plugin_data)
Copy link
Contributor Author

@FuzailBrcm FuzailBrcm Aug 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

lgtm [py/call/wrong-number-class-arguments]

FuzailBrcm
FuzailBrcm commented Aug 10, 2020
View reviewed changes
platform/pddf/platform-api-pddf-base/sonic_platform_pddf_base/pddf_chassis.py

# OPTICs
for index in range(self.platform_inventory['num_ports']):
sfp = Sfp(index, self.pddf_obj, self.plugin_data)
Copy link
Contributor Author

@FuzailBrcm FuzailBrcm Aug 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

lgtm [py/call/wrong-number-class-arguments]

FuzailBrcm
FuzailBrcm commented Aug 10, 2020
View reviewed changes
platform/pddf/platform-api-pddf-base/sonic_platform_pddf_base/pddf_chassis.py

# THERMALs
for i in range(self.platform_inventory['num_temps']):
thermal = Thermal(i, self.pddf_obj, self.plugin_data)
Copy link
Contributor Author

@FuzailBrcm FuzailBrcm Aug 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

lgtm [py/call/wrong-number-class-arguments]

FuzailBrcm
FuzailBrcm commented Aug 10, 2020
View reviewed changes
platform/pddf/platform-api-pddf-base/sonic_platform_pddf_base/pddf_platform.py
raise ValueError

PlatformBase.__init__(self)
self._chassis = Chassis(self.pddf_data, self.pddf_plugin_data)
Copy link
Contributor Author

@FuzailBrcm FuzailBrcm Aug 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

lgtm [py/call/wrong-number-class-arguments]

FuzailBrcm
FuzailBrcm commented Aug 10, 2020
View reviewed changes
platform/pddf/platform-api-pddf-base/sonic_platform_pddf_base/pddf_psu.py
self._fan_list = [] # _fan_list under PsuBase class is a global variable, hence we need to use _fan_list per class instatiation
self.num_psu_fans = int(self.pddf_obj.get_num_psu_fans('PSU{}'.format(index+1)))
for psu_fan_idx in range(self.num_psu_fans):
psu_fan = Fan(0, psu_fan_idx, pddf_data, pddf_plugin_data, True, self.psu_index)
Copy link
Contributor Author

@FuzailBrcm FuzailBrcm Aug 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

lgtm [py/call/wrong-number-class-arguments]

@FuzailBrcm
Copy link
Contributor Author

Added comment
"lgtm[py/call/wrong-number-class-arguments]"
on various lines to ignore this specific alert as they are not correct.

@lgtm-com
Copy link

lgtm-com bot commented Aug 10, 2020

This pull request introduces 8 alerts when merging 7ed4fda0184fb762b12af434ecd12a7f5c59750a into 78baece - view on LGTM.com

new alerts:

  • 7 for Wrong number of arguments in a class instantiation
  • 1 for Syntax error

@lgtm-com
Copy link

lgtm-com bot commented Aug 10, 2020

This pull request introduces 8 alerts when merging 7580b6c78452b402c8ff63fb4ac2ef3fea19805d into 78baece - view on LGTM.com

new alerts:

  • 7 for Wrong number of arguments in a class instantiation
  • 1 for Syntax error

@lgtm-com
Copy link

lgtm-com bot commented Aug 10, 2020

This pull request introduces 7 alerts when merging 3a837d772c70c77086290c6101103fda68f74f92 into 78baece - view on LGTM.com

new alerts:

  • 7 for Wrong number of arguments in a class instantiation

@lgtm-com
Copy link

lgtm-com bot commented Aug 10, 2020

This pull request introduces 7 alerts when merging 92e979096e8609da0b4f74452790980696d5c527 into 2b5e418 - view on LGTM.com

new alerts:

  • 7 for Wrong number of arguments in a class instantiation

@FuzailBrcm
Copy link
Contributor Author

retest vsimage

@FuzailBrcm
Copy link
Contributor Author

retest vsimage please

@FuzailBrcm
Copy link
Contributor Author

Need to ignore the 7 LGTM errors.

These errors are flagged because LGTM is trying to match platform classes developed outside of the PDDF framework with the expectations of the framework, which is not really valid.

@FuzailBrcm FuzailBrcm requested a review from jleveque August 12, 2020 14:35
@ben-gale
Copy link
Collaborator

ben-gale commented Sep 4, 2020

@jleveque - can we get this one merged please?

@ben-gale
Copy link
Collaborator

@jleveque - can we get this one merged please?

@jleveque - Joe?

jleveque
jleveque suggested changes Sep 16, 2020
View reviewed changes
Copy link
Contributor

@jleveque jleveque left a comment

Choose a reason for hiding this comment

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

As comments. Also, please resolve the conflict. Also suggest running all Python files through a tool such as autopep8 (e.g., autopep8 --max-line-length 120) to unify code style.

platform/pddf/i2c/utils/pddf_util.py Outdated
Comment on lines 18 to 26
import os
import commands
import sys, getopt
import logging
import subprocess
import shutil
#import json
import pddfparse
Copy link
Contributor

Choose a reason for hiding this comment

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

Please reorganize imports per PEP8 standards?

Imports should be grouped in the following order, alphabetically sorted within each group:

Standard library imports.
Related third party imports.
Local application/library specific imports.
You should put a blank line between each group of imports.

Also, please limit to one import per line (i.e., split sys and getopt to separate lines). Also remove commented-out imports.

Please do the same for all Python files in the PR.

platform/pddf/i2c/utils/pddf_util.py Outdated
Comment on lines 598 to 607
#print "Enabeling the 'skip_fand' from pmon startup script..."
#if os.path.exists('/usr/share/sonic/platform/pmon_daemon_control.json'):
#with open('/usr/share/sonic/platform/pmon_daemon_control.json','r') as fr:
#data = json.load(fr)
#if 'skip_fand' in data.keys():
#old_val = data['skip_fand']
#if not old_val:
#data['skip_fand'] = True
#with open('/usr/share/sonic/platform/pmon_daemon_control.json','w') as fw:
#json.dump(data,fw)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove all commented code.

platform/pddf/i2c/utils/pddfparse.py Outdated
Comment on lines 210 to 211

##os.system("sleep 1")
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove extra blank line inside function body and commented code. Please do this throughout the PR.

@lgtm-com
Copy link

lgtm-com bot commented Oct 16, 2020

This pull request introduces 8 alerts when merging 32c8657a5f13d3d48116d71c32a827c3916abff8 into 5708e32 - view on LGTM.com

new alerts:

  • 7 for Wrong number of arguments in a class instantiation
  • 1 for 'import *' may pollute namespace

@lgtm-com
Copy link

lgtm-com bot commented Oct 16, 2020

This pull request introduces 9 alerts when merging 83ea1651317d915ba59e6c7d12538468c50bec61 into 5708e32 - view on LGTM.com

new alerts:

  • 7 for Wrong number of arguments in a class instantiation
  • 1 for Syntax error
  • 1 for 'import *' may pollute namespace

@lgtm-com
Copy link

lgtm-com bot commented Oct 16, 2020

This pull request introduces 7 alerts when merging 020ccbb2d020c280408d2d16577f69aa9280d0cb into 5708e32 - view on LGTM.com

new alerts:

  • 7 for Wrong number of arguments in a class instantiation

@FuzailBrcm
Copy link
Contributor Author

@jleveque - I took care of your comments. Can you review?

@FuzailBrcm FuzailBrcm requested a review from jleveque October 16, 2020 15:27
@lgtm-com
Copy link

lgtm-com bot commented Nov 5, 2020

This pull request introduces 7 alerts when merging 42a9c72 into ce6286e - view on LGTM.com

new alerts:

  • 7 for Wrong number of arguments in a class instantiation

@FuzailBrcm
Copy link
Contributor Author

retest vsimage please

@FuzailBrcm
Copy link
Contributor Author

retest vsimage, vs, broadcom please

@FuzailBrcm
Copy link
Contributor Author

retest broadcom please

@FuzailBrcm
Copy link
Contributor Author

retest vs please

@FuzailBrcm
Copy link
Contributor Author

restest vsimage

@FuzailBrcm
Copy link
Contributor Author

restest vsimage please

1 similar comment
@FuzailBrcm
Copy link
Contributor Author

restest vsimage please

@FuzailBrcm
Copy link
Contributor Author

@jleveque
Can you review this please?

@FuzailBrcm
Copy link
Contributor Author

retest vsimage please

@jleveque jleveque merged commit a3dd3f5 into sonic-net:master Nov 12, 2020
santhosh-kt pushed a commit to santhosh-kt/sonic-buildimage that referenced this pull request Feb 25, 2021
@FuzailBrcm @santhosh-kt
Platform Driver Developement Framework (PDDF) (sonic-net#4756)
c94265d 
This change introduces PDDF which is described here: sonic-net/SONiC#536

Most of the platform bring up effort goes in developing the platform device drivers, SONiC platform APIs and validating them. Typically each platform vendor writes their own drivers and platform APIs which is very tailor made to that platform. This involves writing code, building, installing it on the target platform devices and testing. Many of the details of the platform are hard coded into these drivers, from the HW spec. They go through this cycle repetitively till everything works fine, and is validated before upstreaming the code.
PDDF aims to make this platform driver and platform APIs development process much simpler by providing a data driven development framework. This is enabled by:

JSON descriptor files for platform data
Generic data-driven drivers for various devices
Generic SONiC platform APIs
Vendor specific extensions for customisation and extensibility

Signed-off-by: Fuzail Khan <fuzail.khan@broadcom.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jleveque jleveque jleveque approved these changes

Assignees
No one assigned
Labels
Enhancement ➕ PDDF
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@FuzailBrcm @ben-gale @jleveque