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

Platform Driver Development Framework (PDDF): 1) Changes to psu base class (1.0 APIs) to support PDD PSU CLI utils 2) Adding fan_base class to support PDDF fan CLI utils #57

Closed
wants to merge 1 commit into from

Conversation

FuzailBrcm
Copy link
Contributor

  • Adding changes to psu_base class to support PDDF PSU cli utility.
  • Added fan_base class to support PDDF FAN cli utility.

All these changes are w.r.t 1.0 APIs. These will be migrated to 2.0 APIs in future.

…class (1.0 APIs) to support PDDF CLI utils

2) Adding fan_base class to support PDDF fan CLI utils
Copy link
Collaborator

@keboliu keboliu 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

class FanBase(object):
__metaclass__ = abc.ABCMeta

@abc.abstractmethod
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you remove the "abstractmethod" restriction? understood that you are following the convention in other plugin base classes. But with this restriction, it requests all the vendors to implement these functions or at least write some stub functions if they don't want to implement them, otherwise, it will fail when loading the plugins on the vendor's platform. We are in the progress to replace plugins with new platform API, so it's highly possible that some vendors don't want to implement these new plugins, so please consider this.

And I suggest you think about using new platform API instead of adding new plugins, eventually we will use new platform API, put efforts on adding new plugins is kind of waste in my view.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. We will remove @AbstractMethod restriction.
We plan to move to 2.0 APIs eventually. However, we needed this change for short term to support the CLIs in some of our platforms.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with @keboliu -- plugins have been deprecated in favor of the new object-oriented platform 2.0 API. Developing new plugins seems unnecessary as they should no longer be used by platform developers.

@@ -47,3 +47,75 @@ def get_psu_presence(self, index):
"""
return False

def get_model(self, idx):
Copy link
Collaborator

Choose a reason for hiding this comment

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

you may align the method defied in new platform API, if eventually you still want to go with new plugins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to align method names as far as possible, but the methods need index of the PSU.

@FuzailBrcm
Copy link
Contributor Author

keboliu,

Please visit the PR: #62
I have incorporated your comments here.

@keboliu
Copy link
Collaborator

keboliu commented Sep 16, 2019

keboliu,

Please visit the PR: #62
I have incorporated your comments here.

can you close this PR since you created a new one?

@FuzailBrcm FuzailBrcm closed this Sep 16, 2019
@FuzailBrcm
Copy link
Contributor Author

Please visit the PR: #62
I have incorporated the feedback comments there.

oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-platform-common that referenced this pull request Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants