-
Notifications
You must be signed in to change notification settings - Fork 40
Modifications to get_optics to survive from N/A output power value #136
Conversation
try: | ||
float(output_power) | ||
except ValueError: | ||
output_power = -40.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why -40? Is that an insane value that clearly indicates it is not supported/available? I am not an optics person myself :P
In any case, I am fine with having such a value if it makes sense but we should make it clear here:
https://github.com/napalm-automation/napalm-base/blob/develop/napalm_base/base.py#L1332
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was thinking about this. I'm wondering whether it is good to use 0.0 as default value for optical powers as it is (in some cases) completely valid value. So 0.0 doesn't indicate not supported(available) see Cisco 10GBASE SFP+ Modules datasheet table 2 for example. Cisco uses value of -40.0 at Rx side to indicate "no light". Nevermind, as technology evolves somebody will create more sensitive receivers and -40.0 will be again valid value. Should we default to -100 or something?
Even you are fine with such value I think we should have it consistent across platforms and all values of Rx, Tx, instant, max, min, avg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I am fine with -100 as a sort of "infinite" value. Eventually we will move this to the classes in napalm-yang
and those have support to leave empty values.
Before moving forward I'd like @mirceaulinic's opinion though but if he agrees we can probably create a few issues to (1) document the behavior in napalm-base
and (2) update the rest of the drivers as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that makes sense to me.
This looks good to me...so can integrate once we decide default power value for "N/A". |
@kaage Can you update this to make the "infinite" value be -100? |
I will integrate and fix the -100 references. |
Part of: napalm-automation/napalm-base#237