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

Allow omicron to configure per-link tx_eq settings #6914

Merged
merged 1 commit into from
Nov 1, 2024
Merged

Allow omicron to configure per-link tx_eq settings #6914

merged 1 commit into from
Nov 1, 2024

Conversation

Nieuwejaar
Copy link
Contributor

No description provided.

@@ -2355,6 +2355,10 @@ pub struct SwitchPortSettingsView {
/// Link-layer discovery protocol (LLDP) settings.
pub link_lldp: Vec<LldpLinkConfig>,

/// TX equalization settings. These are optional, and most links will not
/// need them.
pub tx_eq: Vec<Option<TxEqConfig>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this collection truly variable in length, or can we expect a fixed length of optional values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Until we add support for breakout connectors and multiple links per port, the length will always be 1. I implemented this as a vector, as that is how all of the other per-link settings are represented in this struct.

Copy link
Contributor

@internet-diglett internet-diglett left a comment

Choose a reason for hiding this comment

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

Hey @Nieuwejaar, I attached a few questions in order to get a better understand of how this feature is going to be used. Will we need to expose this to customers via the external api?

Comment on lines +2358 to +2362
/// TX equalization settings. These are optional, and most links will not
/// need them.
Copy link
Contributor

Choose a reason for hiding this comment

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

When will links need this setting? Is there an issue / document we can reference for this feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed for specific models of transceiver which don't autotune and don't work with the normal default settings. So far, we've only found one of those, and that is now hardcoded in dendrite. For now, this is for internal use only, by whoever is doing the initial customer install.

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.

2 participants