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] Accton add to support as4630-54te platform. #6683

Merged
merged 6 commits into from
Feb 25, 2021
Merged

[Platform] Accton add to support as4630-54te platform. #6683

merged 6 commits into from
Feb 25, 2021

Conversation

ec-michael-shih
Copy link
Contributor

@ec-michael-shih ec-michael-shih commented Feb 4, 2021

- Why I did it
My company(Accton) have a new model names: as4630-54te.
- How I did it
This pull request is based on as4630-54pe, so I reference as4630-54pe to create new model: as4630-54te
- How to verify it
Fan monitor is work, when execute the command: [accton_as4630_54te_util.py install], then the fan speed will come down.
SFP can detect module present, log show as follow:
=====================
...(skip)....
Ethernet48 Present
Ethernet49 Not present
Ethernet50 Present
Ethernet51 Not present
Ethernet52 Present
Ethernet56 Not present
=====================
PSU can detect the status:
=====================
root@sonic:/# psuutil status
PSU Status

  PSU 1 NOT PRESENT
  PSU 2 OK

Can get the info from eeprom:
=====================
root@sonic:/# show platform syseeprom
Warning: failed to retrieve PORT table from ConfigDB!
Warning: failed to retrieve PORT table from ConfigDB!
TlvInfo Header:
Id String:    TlvInfo
Version:      1
Total Length: 175
TLV Name             Code Len Value
Product Name         0x21  16 4630-54TE-O-AC-F
Part Number          0x22  13 F0PZZ4654043A
Serial Number        0x23  15 463054TE2102018
Base MAC Address     0x24   6 90:3C:B3:7D:52:40
Manufacture Date     0x25  19 01/14/2021 17:13:53
Label Revision       0x27   4 R01B
Platform Name        0x28  28 x86_64-accton_as4630_54pe-r0
ONIE Version         0x29  13 2020.08.00.03
MAC Addresses        0x2A   2 256
Manufacturer         0x2B   6 Accton
Manufacture Country  0x2C   2 TW
Vendor Name          0x2D   8 Edgecore
Diag Version         0x2E  11 01.01.01.00
CRC-32               0xFE   4 0x660EA8B7
(checksum valid)

- Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012

- Description for the changelog

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

@lgtm-com
Copy link

lgtm-com bot commented Feb 4, 2021

This pull request introduces 61 alerts when merging 638a205 into e387531 - view on LGTM.com

new alerts:

  • 41 for Unused import
  • 8 for Unused local variable
  • 6 for Use of 'global' at module level
  • 2 for Unreachable code
  • 2 for Variable defined multiple times
  • 1 for Except block handles 'BaseException'
  • 1 for Duplicate key in dict literal

@@ -0,0 +1,23 @@
#!/usr/bin/env python
Copy link
Contributor

Choose a reason for hiding this comment

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

These platform plugins have been deprecated in favor of the new sonic-platform package. Please refer to the documentation here: https://github.com/Azure/SONiC/wiki/Porting-Guide#sonic-platform-api-package

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe "show version" still uses "decode-syseeprom" which loads this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the introduction of syseepromd, the system EEPROM contents should be collected by syseepromd and stored to State DB, and decode-syseeprom should simply pull the values from there.

It does appear that in show version the call is made to sudo decode-syseeprom -s, which will attempt to access the EEPROM directly. Instead, the call should probably be changed to sudo decode-syseeprom -d -s to read the serial number from the DB.

Also, decode-syseeprom should be refactored to use the new sonic-platform package for direct access rather than the old eeprom.py plugin. I have created the following issue to track this improvement: sonic-net/sonic-utilities#1404

Copy link
Contributor

Choose a reason for hiding this comment

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

I have refactored decode-syseeprom to utilize the sonic-platform package via sonic-net/sonic-utilities#1435.

I have also done the same for psuutil and sfputil.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be safe to remove these plugins now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Goit it, I'll remove it.

@@ -0,0 +1,62 @@
#!/usr/bin/env python
Copy link
Contributor

Choose a reason for hiding this comment

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

These platform plugins have been deprecated in favor of the new sonic-platform package. Please refer to the documentation here: https://github.com/Azure/SONiC/wiki/Porting-Guide#sonic-platform-api-package

@@ -0,0 +1,196 @@
# sfputil.py
Copy link
Contributor

Choose a reason for hiding this comment

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

These platform plugins have been deprecated in favor of the new sonic-platform package. Please refer to the documentation here: https://github.com/Azure/SONiC/wiki/Porting-Guide#sonic-platform-api-package

@ec-michael-shih
Copy link
Contributor Author

Dear Sonic community manager:
The platform: as4630-54te has already at sonic-platform(API2.0)
Please help to check and commit, thank you very much.
//michael

@lgtm-com
Copy link

lgtm-com bot commented Feb 19, 2021

This pull request introduces 52 alerts when merging cdc1291 into 5a49a0f - view on LGTM.com

new alerts:

  • 32 for Unused import
  • 8 for Unused local variable
  • 6 for Use of 'global' at module level
  • 2 for Unreachable code
  • 2 for Variable defined multiple times
  • 1 for Except block handles 'BaseException'
  • 1 for Duplicate key in dict literal

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.

Please fix LGTM alerts.

@lgtm-com
Copy link

lgtm-com bot commented Feb 23, 2021

This pull request introduces 6 alerts when merging f23935c into 7ec4d15 - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 2 for Variable defined multiple times
  • 1 for Unused local variable
  • 1 for Unreachable code

@lgtm-com
Copy link

lgtm-com bot commented Feb 23, 2021

This pull request introduces 1 alert when merging 1bfc9bc into f77157f - view on LGTM.com

new alerts:

  • 1 for Unreachable code

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 remove plugins/eeprom.py, plugins/psuutil.py and plugins/sfputil.py, as they should be no longer necessary.

@@ -0,0 +1,121 @@
#############################################################################
# Celestica
Copy link
Contributor

Choose a reason for hiding this comment

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

Celestica? Copy/paste issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is copy/paste issue. I'll remove the keyword.

SYSFS_PATH = "/sys/bus/i2c/devices"

def __init__(self, thermal_index=0):
self.index = thermal_index
Copy link
Contributor

Choose a reason for hiding this comment

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

Please call the superclass initializer here for completeness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for review, I'll modify it.

@ec-michael-shih
Copy link
Contributor Author

As comments.

Also, please remove plugins/eeprom.py, plugins/psuutil.py and plugins/sfputil.py, as they should be no longer necessary.

My testing code is Mon Feb 8 08:07:33 2021 -0800
And I test [sensors] command will call into plugins/eeprom.py now.
Still need to remove plugin this directory now?!

@jleveque
Copy link
Contributor

As comments.
Also, please remove plugins/eeprom.py, plugins/psuutil.py and plugins/sfputil.py, as they should be no longer necessary.

My testing code is Mon Feb 8 08:07:33 2021 -0800
And I test [sensors] command will call into plugins/eeprom.py now.
Still need to remove plugin this directory now?!

The sensors command calls into plugins/eeprom.py? Can you show me where?

@ec-michael-shih
Copy link
Contributor Author

As comments.
Also, please remove plugins/eeprom.py, plugins/psuutil.py and plugins/sfputil.py, as they should be no longer necessary.

My testing code is Mon Feb 8 08:07:33 2021 -0800
And I test [sensors] command will call into plugins/eeprom.py now.
Still need to remove plugin this directory now?!

The sensors command calls into plugins/eeprom.py? Can you show me where?

My fault!! The command: show platform syseeprom will call into plugin/eeprom.py

try:
print('456')
from sonic_eeprom import eeprom_tlvinfo

except ImportError as e:
raise ImportError(str(e) + "- required module not found")

class board(eeprom_tlvinfo.TlvInfoDecoder):
_TLV_INFO_MAX_LEN = 256
print('123')

def __init__(self, name, path, cpld_root, ro):
    self.eeprom_path = "/sys/bus/i2c/devices/1-0057/eeprom"
    super(board, self).__init__(self.eeprom_path, 0, '', True)

log:

root@sonic:/# show platform syseeprom
456
123
TlvInfo Header:
Id String: TlvInfo
Version: 1
Total Length: 175
TLV Name Code Len Value


Product Name 0x21 16 4630-54TE-O-AC-F
Part Number 0x22 13 F0PZZ4654043A
Manufacture Date 0x25 19 01/14/2021 17:13:53
Label Revision 0x27 4 R01B
Platform Name 0x28 28 x86_64-accton_as4630_54pe-r0
ONIE Version 0x29 13 2020.08.00.03
MAC Addresses 0x2A 2 256
Manufacturer 0x2B 6 Accton
Manufacture Country 0x2C 2 TW
Vendor Name 0x2D 8 Edgecore
Diag Version 0x2E 11 01.01.01.00
Serial Number 0x23 15 463054TE2102018
Base MAC Address 0x24 6 90:3C:B3:7D:52:40
CRC-32 0xFE 4 0x660EA8B7
(checksum valid)

@jleveque
Copy link
Contributor

jleveque commented Feb 24, 2021

I have refactored the utilities to no longer use the old plugins:

However, the sonic-utilities submodule has not yet been updated to reflect these changes in sonic-buildimage. I will do that soon.

@ec-michael-shih
Copy link
Contributor Author

I have refactored the utilities to no longer use the old plugins:

* [Azure/sonic-utilities#1435](https://github.com/Azure/sonic-utilities/pull/1435)

* [Azure/sonic-utilities#1434](https://github.com/Azure/sonic-utilities/pull/1434)

* [Azure/sonic-utilities#1421](https://github.com/Azure/sonic-utilities/pull/1421)

However, the sonic-utilities submodule has not yet been updated to reflect these changes in sonic-buildimage. I will do that soon.

Got it.
So, your suggestion still prefer remove plugin(directory) in this PR?

@jleveque
Copy link
Contributor

jleveque commented Feb 24, 2021

Got it.
So, your suggestion still prefer remove plugin(directory) in this PR?

I would prefer it, yes. If you would like to wait until I update the submodule so that you can test again, please let me know. I just opened a PR to update the submodule here.

@ec-michael-shih
Copy link
Contributor Author

Got it.
So, your suggestion still prefer remove plugin(directory) in this PR?

I would prefer it, yes. If you would like to wait until I update the submodule so that you can test again, please let me know. I just opened a PR to update the submodule here.

By the way,
our company will face a situation where customers will develop their functions on the old branch(ex: 202012, 202006).
And this PR wants to merge into these old branch.
So is it possible to keep the plugin folder? or .... any suggestion?!

@jleveque
Copy link
Contributor

By the way,
our company will face a situation where customers will develop their functions on the old branch(ex: 202012, 202006).
And this PR wants to merge into these old branch.
So is it possible to keep the plugin folder? or .... any suggestion?!

I didn't realize you intended to backport this PR to earlier release branches, because none of the boxes are checked in the PR description. If that is the case, then we can go ahead and keep the plugins here, and we can delete them from the master branch in the future.

@ec-michael-shih
Copy link
Contributor Author

By the way,
our company will face a situation where customers will develop their functions on the old branch(ex: 202012, 202006).
And this PR wants to merge into these old branch.
So is it possible to keep the plugin folder? or .... any suggestion?!

I didn't realize you intended to backport this PR to earlier release branches, because none of the boxes are checked in the PR description. If that is the case, then we can go ahead and keep the plugins here, and we can delete them from the master branch in the future.

Thank you for your suggestion.
It's Good!!! to merger this PR into backport(include /plugins folder) on 202006 and 202012 branch.
And for master branch, need to bother you to delete /plugins folder in the future.

@jleveque jleveque merged commit 66e3e51 into sonic-net:master Feb 25, 2021
yxieca pushed a commit that referenced this pull request Mar 4, 2021
Add support for Accton as4630-54te platform
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
lolyu pushed a commit to lolyu/sonic-buildimage that referenced this pull request Sep 13, 2021
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