Xcvrd Refactor 3/13: Breakup task_worker into separate functions - 1 #701
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
mihirpat1
left a comment
mihirpat1
left a comment
There was a problem hiding this comment.
@bobby-nexthop Can you please fix the test failure?
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the task_worker method by extracting the per-port processing logic into a new process_single_lport method. The main change improves code organization and maintainability by separating concerns.
- Extracted port processing logic from
task_workerinto a newprocess_single_lportmethod - Replaced all
continuestatements withreturnstatements to match the new function structure - Moved
gearbox_lanes_dictinitialization from loop-level to function-level
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…of once per iteration over all ports.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| if self.task_stopping_event.is_set(): | ||
| break | ||
|
|
||
| if lport not in self.port_dict: |
There was a problem hiding this comment.
@bobby-nexthop Wondering in what scenario will self.port_dict not contain lport?
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # A retry should always start over at INSETRTED state, while the | ||
| # expiration will reset the state to INSETRTED and advance the |
There was a problem hiding this comment.
Spelling error: "INSETRTED" should be "INSERTED".
| # A retry should always start over at INSETRTED state, while the | |
| # expiration will reset the state to INSETRTED and advance the | |
| # A retry should always start over at INSERTED state, while the | |
| # expiration will reset the state to INSERTED and advance the |
| # A retry should always start over at INSETRTED state, while the | ||
| # expiration will reset the state to INSETRTED and advance the |
There was a problem hiding this comment.
Spelling error: "INSETRTED" should be "INSERTED".
| # A retry should always start over at INSETRTED state, while the | |
| # expiration will reset the state to INSETRTED and advance the | |
| # A retry should always start over at INSERTED state, while the | |
| # expiration will reset the state to INSERTED and advance the |
| self.force_cmis_reinit(lport, retries + 1) | ||
| return | ||
|
|
||
| # Ensure the Datapath is NOT Activated unless the host Tx siganl is good. |
There was a problem hiding this comment.
Spelling error: "siganl" should be "signal".
| # Ensure the Datapath is NOT Activated unless the host Tx siganl is good. | |
| # Ensure the Datapath is NOT Activated unless the host Tx signal is good. |
| if self.is_decomm_lead_lport(lport): | ||
| # Set all the DP lanes AppSel to unused(0) when non default app code needs to be configured | ||
| self.port_dict[lport]['appl'] = appl = 0 | ||
| self.port_dict[lport]['host_lanes_mask'] = host_lanes_mask = self.ALL_LANES_MASK |
There was a problem hiding this comment.
Variable host_lanes_mask is not used.
| self.port_dict[lport]['host_lanes_mask'] = host_lanes_mask = self.ALL_LANES_MASK | |
| self.port_dict[lport]['host_lanes_mask'] = self.ALL_LANES_MASK |
|
@bobby-nexthop Can you please help to close the open comments? |
3 participants
Description
No logic changes. This change looks like a huge diff but its just moving all of the CMIS logic into a new function process_single_lport. This is needed to make the following diffs with logic split up more readable
Motivation and Context
xcvrd has gotten to 4000 lines long. To make things easier, we'd like to refactor it. This is the second PR in a series that aims to do the following:
The new flow will go from calling
task_worker()toHow Has This Been Tested?
Additional Information (Optional)