Add OC Event Counter #597
Open
Add OC Event Counter #597
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis pull request adds a display for overcurrent events to the jtop tool. It introduces new functions to find and update overcurrent event counters, and integrates this information into the power display. Sequence diagram for overcurrent event monitoringsequenceDiagram
participant PS as PowerService
participant FS as FileSystem
participant GUI as GUI Display
PS->>FS: find_all_oc_event_counters()
FS-->>PS: event_counter_files
loop For each counter
PS->>FS: Read counter value
FS-->>PS: counter_value
end
PS->>GUI: Update display with OC events
Note over GUI: Display count with color:
Note over GUI: Red if throttling
Note over GUI: Yellow if count > 0
Note over GUI: Green if count = 0
Class diagram showing PowerService modificationsclassDiagram
class PowerService {
-dict _power_sensor
-dict _power_avg
-dict _oc_event_counts
+__init__()
+get_status()
}
class PowerFunctions {
+find_all_oc_event_counters()
+update_oc_event_counts(event_counts)
+find_all_i2c_power_monitor(i2c_path)
+read_power_status(data)
}
PowerService ..> PowerFunctions: uses
State diagram for OC event counter displaystateDiagram-v2
[*] --> NoEvents: Initial State
NoEvents --> ActiveEvents: OC Event Detected
ActiveEvents --> Throttling: New OC Event
Throttling --> ActiveEvents: Throttling Ends
ActiveEvents --> NoEvents: Reset
state NoEvents {
[*] --> GreenDisplay: Count = 0
}
state ActiveEvents {
[*] --> YellowDisplay: Count > 0
}
state Throttling {
[*] --> RedDisplay: Is Throttling
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @sgillen - I've reviewed your changes - here's some feedback:
Overall Comments:
- Additional testing across different Jetson hardware configurations should be completed before merging, as noted in the TODO.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+121
to
+123
| color = NColors.red() if is_throttling else (NColors.yellow() if oc_event_cnt > 0 else NColors.green()) | ||
| stdscr.addstr(pos_y + len_power + 3, center_x - column_power - 5, "OC EVENT COUNT: ", curses.A_BOLD) | ||
| stdscr.addstr(pos_y + len_power + 3, center_x + 2, str(oc_event_cnt), curses.A_BOLD | color) |
There was a problem hiding this comment.
suggestion: Consider extracting duplicated OC event display logic into a helper function
This logic appears in both compact_power and control_power. A helper would reduce duplication and make future updates easier.
Suggested implementation:
# If there are no OC events or no space, return
if not jetson.power['oc_events'] or len_power + 3 >= height:
return len(power) + 1
return display_oc_events(stdscr, jetson.power['oc_events'], pos_y, len_power, center_x, column_power)
def display_oc_events(stdscr, oc_events, pos_y, len_power, center_x, column_power):
"""Helper function to display OC events with appropriate coloring"""
oc_event_cnt = oc_events['count']
is_throttling = oc_events['is_throttling']
# Set color based on throttling status and event count
color = NColors.red() if is_throttling else (NColors.yellow() if oc_event_cnt > 0 else NColors.green())
# Display OC event count
stdscr.addstr(pos_y + len_power + 3, center_x - column_power - 5, "OC EVENT COUNT: ", curses.A_BOLD)
stdscr.addstr(pos_y + len_power + 3, center_x + 2, str(oc_event_cnt), curses.A_BOLD | color)
return len_power + 3You'll need to:
- Make sure the NColors and curses imports are available in the scope where the helper function is defined
- Update any other locations in the codebase that display OC events to use this new helper function
- Consider adding this helper function to a utilities module if it might be useful in other parts of the application
Comment on lines
+334
to
+343
| # If there are OC counters, update those as well | ||
| oc_events = {} | ||
| if self._oc_event_counts: | ||
| oc_events['is_throttling'] = update_oc_event_counts(self._oc_event_counts) | ||
| oc_events['count'] = 0 | ||
| # Sum up all the events: | ||
| for filename, count in self._oc_event_counts.items(): | ||
| oc_events['count'] += count | ||
|
|
||
| return {'rail': rails, 'tot': total, 'oc_events': oc_events} |
There was a problem hiding this comment.
suggestion: Consider only including 'oc_events' in the return dict when OC counters are present
This would make the API more consistent and avoid clients needing to check both for key existence and empty dict.
Suggested change
| # If there are OC counters, update those as well | |
| oc_events = {} | |
| if self._oc_event_counts: | |
| oc_events['is_throttling'] = update_oc_event_counts(self._oc_event_counts) | |
| oc_events['count'] = 0 | |
| # Sum up all the events: | |
| for filename, count in self._oc_event_counts.items(): | |
| oc_events['count'] += count | |
| return {'rail': rails, 'tot': total, 'oc_events': oc_events} | |
| # Build return dictionary | |
| ret_dict = {'rail': rails, 'tot': total} | |
| # Only include OC events if counters exist | |
| if self._oc_event_counts: | |
| oc_events = { | |
| 'is_throttling': update_oc_event_counts(self._oc_event_counts), | |
| 'count': sum(self._oc_event_counts.values()) | |
| } | |
| ret_dict['oc_events'] = oc_events | |
| return ret_dict |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a display for overcurrent events, OC events cause severe throttling of CPU/GPU clocks, and often users don't realize this at all.
TODO - still needs additional testing under load and outside of AGX Orin.
Summary by Sourcery
Add a display for overcurrent events, including the total count and an indicator for current throttling status.
New Features:
Tests: