-
Notifications
You must be signed in to change notification settings - Fork 64
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
Support jetson power fan configs #2379
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
9ec45a7
Add tests for ConfigJsonConfigBackend
cywang117 54fcfa2
Support "os" key with object values in ConfigJsonConfigBackend
cywang117 828bd22
Add PowerFanConfig config backend
cywang117 2f2b2e1
Don't require reboot if setting fan control
cywang117 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,170 @@ | ||
import { isRight } from 'fp-ts/lib/Either'; | ||
import Reporter from 'io-ts-reporters'; | ||
import * as t from 'io-ts'; | ||
import * as _ from 'lodash'; | ||
|
||
import { ConfigBackend } from './backend'; | ||
import type { ConfigOptions } from './backend'; | ||
import { schemaTypes } from '../schema-type'; | ||
import log from '../../lib/supervisor-console'; | ||
import * as constants from '../../lib/constants'; | ||
|
||
type ConfigJsonBackend = { | ||
get: (key: 'os') => Promise<unknown>; | ||
set: (opts: { os: Record<string, any> }) => Promise<void>; | ||
}; | ||
|
||
/** | ||
* A backend to handle Jetson power and fan control | ||
* | ||
* Supports: | ||
* - {BALENA|RESIN}_HOST_CONFIG_power_mode = "low" | "mid" | "high" | "default" |"$MODE_ID" | ||
* - {BALENA|RESIN}_HOST_CONFIG_fan_profile = "quiet" | "cool" | "default" |"$MODE_ID" | ||
*/ | ||
export class PowerFanConfig extends ConfigBackend { | ||
private static readonly CONFIGS = new Set(['power_mode', 'fan_profile']); | ||
private static readonly PREFIX = `${constants.hostConfigVarPrefix}CONFIG_`; | ||
private static readonly SCHEMA = t.exact( | ||
t.partial({ | ||
power: t.exact( | ||
t.partial({ | ||
mode: t.string, | ||
}), | ||
), | ||
fan: t.exact( | ||
t.partial({ | ||
profile: t.string, | ||
}), | ||
), | ||
}), | ||
); | ||
|
||
private readonly configJson: ConfigJsonBackend; | ||
public constructor(configJson: ConfigJsonBackend) { | ||
super(); | ||
this.configJson = configJson; | ||
} | ||
|
||
public static stripPrefix(name: string): string { | ||
if (!name.startsWith(PowerFanConfig.PREFIX)) { | ||
return name; | ||
} | ||
return name.substring(PowerFanConfig.PREFIX.length); | ||
} | ||
|
||
public async matches(deviceType: string): Promise<boolean> { | ||
// We only support Jetpack 6 devices for now, which includes all Orin devices | ||
// except for jetson-orin-nx-xv3 which is still on Jetpack 5 as of OS v5.1.36 | ||
return new Set([ | ||
'jetson-agx-orin-devkit', | ||
'jetson-agx-orin-devkit-64gb', | ||
'jetson-orin-nano-devkit-nvme', | ||
'jetson-orin-nano-seeed-j3010', | ||
'jetson-orin-nx-seeed-j4012', | ||
'jetson-orin-nx-xavier-nx-devkit', | ||
]).has(deviceType); | ||
} | ||
|
||
public async getBootConfig(): Promise<ConfigOptions> { | ||
// Get raw config.json contents | ||
let rawConf: unknown; | ||
try { | ||
rawConf = await this.configJson.get('os'); | ||
} catch (e: unknown) { | ||
log.error( | ||
`Failed to read config.json while getting power / fan configs: ${(e as Error).message ?? e}`, | ||
); | ||
return {}; | ||
} | ||
|
||
// Decode to power fan schema from object type, filtering out unrelated values | ||
const powerFanConfig = PowerFanConfig.SCHEMA.decode(rawConf); | ||
|
||
if (isRight(powerFanConfig)) { | ||
const conf = powerFanConfig.right; | ||
return { | ||
...(conf.power?.mode != null && { | ||
power_mode: conf.power.mode, | ||
}), | ||
...(conf.fan?.profile != null && { | ||
fan_profile: conf.fan.profile, | ||
}), | ||
}; | ||
} else { | ||
return {}; | ||
} | ||
} | ||
|
||
public async setBootConfig(opts: ConfigOptions): Promise<void> { | ||
// Read raw configs for "os" key from config.json | ||
let rawConf; | ||
try { | ||
rawConf = await this.configJson.get('os'); | ||
} catch (err: unknown) { | ||
log.error(`${(err as Error).message ?? err}`); | ||
return; | ||
} | ||
|
||
// Decode to "os" object type while leaving in unrelated values | ||
const maybeCurrentConf = schemaTypes.os.type.decode(rawConf); | ||
if (!isRight(maybeCurrentConf)) { | ||
log.error( | ||
'Failed to decode current os config:', | ||
Reporter.report(maybeCurrentConf), | ||
); | ||
return; | ||
} | ||
// Current config could be undefined if there's no os key in config.json, so default to empty object | ||
const conf = maybeCurrentConf.right ?? {}; | ||
|
||
// Update or delete power mode | ||
if ('power_mode' in opts) { | ||
conf.power = { | ||
mode: opts.power_mode, | ||
}; | ||
} else { | ||
delete conf?.power; | ||
} | ||
|
||
// Update or delete fan profile | ||
if ('fan_profile' in opts) { | ||
conf.fan = { | ||
profile: opts.fan_profile, | ||
}; | ||
} else { | ||
delete conf?.fan; | ||
} | ||
|
||
await this.configJson.set({ os: conf }); | ||
} | ||
|
||
public isSupportedConfig = (name: string): boolean => { | ||
return PowerFanConfig.CONFIGS.has(PowerFanConfig.stripPrefix(name)); | ||
}; | ||
|
||
public isBootConfigVar(envVar: string): boolean { | ||
return PowerFanConfig.CONFIGS.has(PowerFanConfig.stripPrefix(envVar)); | ||
} | ||
|
||
public async isRebootRequired(opts: ConfigOptions): Promise<boolean> { | ||
const supportedOpts = _.pickBy( | ||
_.mapKeys(opts, (_value, key) => PowerFanConfig.stripPrefix(key)), | ||
(_value, key) => this.isSupportedConfig(key), | ||
); | ||
const current = await this.getBootConfig(); | ||
// A reboot is only required if the power mode is changing | ||
return current.power_mode !== supportedOpts.power_mode; | ||
} | ||
|
||
public processConfigVarName(envVar: string): string { | ||
return PowerFanConfig.stripPrefix(envVar).toLowerCase(); | ||
} | ||
|
||
public processConfigVarValue(_key: string, value: string): string { | ||
return value; | ||
} | ||
|
||
public createConfigVarName(name: string): string | null { | ||
return `${PowerFanConfig.PREFIX}${name}`; | ||
} | ||
} |
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
Why was it decided to bother with this? Before, all host config changes would require a reboot. Even if it is not necessary for the fan profile, it doesn't seem worth it the extra complexity just to avoid a reboot in this particular case.
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.
Rebooting when only changing the fan control runtime config is bad UX for the user as it causes unnecessary disruption to devices as well as storage wear. A host config being tied to something that requires a reboot isn't a very strong tie in my opinion; there certainly exists the concept of a runtime OS configuration even if we don't support it in the Supervisor yet. Some examples of this include swappiness and other kernel runtime parameters via
sysctl
, network configuration such as iptables, and hardware-specific runtime configs: motor speed, sensor sensitivity, etc..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.
Why does it cause storage wear? How often do we expect users to be changing the fan control speed?
I agree that there are use cases for changing configurations without a reboot.
I don't think the config variable interface is great for that. Config variables require an API call and a target state pull before the changes are applied, is not an interface made for runtime requirements.
If there is need for such an interface then we should work on that rather than trying to fit the config var interface for a specific use case
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.
Agreed, I think this feature has exposed some limits of the way we handle configs on top of what we were aware of before.