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

[sonic_eeprom] Class methods shouldn't take the EEPROM data as a parameter #170

Open
jleveque opened this issue Feb 13, 2021 · 11 comments · May be fixed by #190
Open

[sonic_eeprom] Class methods shouldn't take the EEPROM data as a parameter #170

jleveque opened this issue Feb 13, 2021 · 11 comments · May be fixed by #190

Comments

@jleveque
Copy link
Contributor

EEPROM class should obtain EEPROM data when instantiated and store as a class member, then use that data in methods rather than having to pass the raw data into the methods. It defeats the purpose of object-orientation.

@aggpiyush
Copy link

Hi, I'd like to take this issue. It's a requirement of my course and therefore, I'll make sure to resolve it. Can you please assign it to me?

@jleveque
Copy link
Contributor Author

jleveque commented Apr 4, 2021

@Piyushagg19: Thank you for volunteering! I have assigned the issue to you. If you have any questions along the way, please feel free to discuss here.

@aggpiyush
Copy link

@jleveque: I went through the 'eeprom_base' file but I'm not sure if I understand the issue completely. Can you please explain the issue a bit more or provide me with an example. Also, if you could direct me to the file that needs the necessary changes, it would be great.

@jleveque
Copy link
Contributor Author

@Piyushagg19: You'll notice that the open_eeprom() method returns a file handle to the EEPROM, which is in turn passed back into most of the methods in the class. It would be better if open_eeprom() returned a boolean value and stored the open file handle internally in the class, preventing the need for the caller to store it and pass it back in to other methods.

@aggpiyush
Copy link

aggpiyush commented May 5, 2021

@jleveque: Hi. Are there any test scripts available that I can run to verify the changes made? There are no instructions in the contributing guide regarding tests.

@jleveque
Copy link
Contributor Author

jleveque commented May 5, 2021

@Piyushagg19: You should be able to use the decode-syseeprom utility for testing your changes.

@aggpiyush
Copy link

@jleveque: Can you please guide me on how to use it? This is new for me. Thanks!

@jleveque
Copy link
Contributor Author

jleveque commented May 5, 2021

Log into a SONiC device and run sudo decode-syseeprom --help:

admin@sonic~$ sudo decode-syseeprom --help
Usage: /usr/local/bin/decode-syseeprom [-s][-m]

Options:
  -h, --help  show this help message and exit
  -d          print eeprom from database
  -s          print device serial number/service tag
  -p          print the device product name
  -m          print the base mac address for management interfaces

Running sudo decode-syseeprom will read info directly from the EEPROM, sudo decode-syseeprom -d will read cached EEPROM data from our Redis DB.

@aggpiyush
Copy link

@jleveque: I'm afraid I don't have access to SONiC device. Can this utility test be performed online or on a virtual machine using Windows machine and qemu or any other way possible?

@jleveque
Copy link
Contributor Author

jleveque commented May 6, 2021

Unfortunately, a virtual switch doesn't have an EEPROM, and we currently are not mocking one, so you would need access to a physical switch to work on this.

@aggpiyush
Copy link

aggpiyush commented May 31, 2021

@jleveque: I have created a Pull Request and it has passed all the checks. Can you please let me know how to propagate with the review process?

oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-platform-common that referenced this issue Oct 25, 2024
…in log (sonic-net#170)

Changed the chassis definition to be a part of the class do the chassis will be found in run() function

Without the change, an error appears in syslog:
`ERR pmon#thermalctld: Caught exception while running thermal policy - NameError("name 'chassis' is not defined")`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants