sonic-platform-common icon indicating copy to clipboard operation
sonic-platform-common copied to clipboard

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

Open jleveque opened this issue 4 years ago • 11 comments

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.

jleveque avatar Feb 13 '21 02:02 jleveque

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?

aggpiyush avatar Apr 04 '21 18:04 aggpiyush

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

jleveque avatar Apr 04 '21 19:04 jleveque

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

aggpiyush avatar Apr 12 '21 21:04 aggpiyush

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

jleveque avatar Apr 12 '21 21:04 jleveque

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

aggpiyush avatar May 05 '21 19:05 aggpiyush

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

jleveque avatar May 05 '21 19:05 jleveque

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

aggpiyush avatar May 05 '21 23:05 aggpiyush

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.

jleveque avatar May 05 '21 23:05 jleveque

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

aggpiyush avatar May 06 '21 19:05 aggpiyush

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.

jleveque avatar May 06 '21 20:05 jleveque

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

aggpiyush avatar May 31 '21 05:05 aggpiyush