Page MenuHomeFreeBSD

Make vt(4) palette configurable
ClosedPublic

Authored by gonzo on Dec 26 2017, 9:33 PM.

Details

Summary

vt(4) uses hardcoded values for standard 16-colors VGA palette
and the only way to modify it at the monent is by patching kernel.
This patch introduces set of loader variables that can be set
to alter pallete values. Variable name pattern is kern.vt.color.N.rgb
where N is a pallete index varying from 0 to 15. The value is
either coma-separated rgb triplet with values in a range of 0 to 255,
i.e. "255,255,255". Or HTML-like hex-triple, i.e: "#ffffff".

These values can be configured in loader.conf(5)

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

gonzo created this revision.Dec 26 2017, 9:33 PM
ray accepted this revision.Dec 27 2017, 8:47 AM

Looks cute :)

This revision is now accepted and ready to land.Dec 27 2017, 8:47 AM
manu added a subscriber: manu.Dec 27 2017, 8:54 AM

It should be documented in loader.conf(5), otherwise looks okay.

gonzo updated this revision to Diff 37172.Dec 29 2017, 1:08 AM

Document new tunable in vt(4)

This revision now requires review to proceed.Dec 29 2017, 1:08 AM
gonzo added a comment.Dec 29 2017, 1:10 AM
In D13645#285283, @manu wrote:

It should be documented in loader.conf(5), otherwise looks okay.

I checked loader.conf it doesn't have much of vt specific info, looks like all tunables are in vt(4). So I put description there

bcr added a subscriber: bcr.Dec 29 2017, 1:22 PM

Typo fix.

share/man/man4/vt.4
214 ↗(On Diff #37172)

s/coma/comma/

jilles requested changes to this revision.Dec 29 2017, 2:28 PM
jilles added a subscriber: jilles.

Looks useful.

sys/dev/vt/colors/vt_termcolors.c
102 ↗(On Diff #37172)

Hmm, perhaps supporting only #rrggbb is sufficient; this reduces verbose and error-prone code. If so, ignore my comments about the r,g,b parser below :)

103 ↗(On Diff #37172)

Given that strtoul() depends on the '\0', passing along the length may not be very useful.

112 ↗(On Diff #37172)

Given that '\0' is not a space and not a comma, the condition on the length is redundant.

Also, it seems unusual to redundantly parenthesize arithmetic within a relational operator or relational operators within logical operators.

139 ↗(On Diff #37172)

Note that any characters after the blue value are ignored. Is this deliberate?

156–159 ↗(On Diff #37172)

Perhaps change color_def from 0..100 to 0..255, since it is an implementation detail of vt_termcolors.c. This makes the code slightly easier to understand and improves accuracy.

This revision now requires changes to proceed.Dec 29 2017, 2:28 PM
gonzo marked 3 inline comments as done.Dec 29 2017, 9:40 PM
gonzo added inline comments.
sys/dev/vt/colors/vt_termcolors.c
102 ↗(On Diff #37172)

I think having comma-separated triplets is useful. Normally people want to migrate their color scheme from whatever format they have and it's either hex triplets or rgb triplets (in my experience). Supporting both formats may save some users' time.

139 ↗(On Diff #37172)

Nope, it's oversight from my side. I added this check and some fixes to the parser. I didn't want to make "r,g.b" parser too strict and enforce comma-only format, hence the whitespaces check. But there is no reason to ignore garbage at the end of the string

156–159 ↗(On Diff #37172)

It makes sense to have color_def in percentages because it may be applied to different bits-per-pixel values. On the other hand most common format from user perspective is 8-bit color component specification. IMO it's better to convert from user visible data to internal representation than make palette application logic less readable.

gonzo updated this revision to Diff 37198.Dec 29 2017, 9:40 PM
  • Fixed typo in man page
  • Addressed jilles@ comments in rgb-triplet parsing code
jilles added inline comments.Dec 29 2017, 9:45 PM
sys/dev/vt/colors/vt_termcolors.c
156–159 ↗(On Diff #37172)

Provided there is no overflow, the maximum of 100 or 255 simply appears in #define CF below. This is just a bit of churn of the color_def table.

gonzo added inline comments.Dec 30 2017, 2:19 AM
sys/dev/vt/colors/vt_termcolors.c
156–159 ↗(On Diff #37172)

I think switching from 100 to 255 is a marginal improvement and not strictly required for the configurable palette feature. What IMO would be better from accuracy point of view is switching to 0..255 range in a palette and using number of bits per color component and logical shift instead of Nmax and dividing by non-power of two numbers. I believe this should be done as a separate patch

gonzo added inline comments.Dec 30 2017, 2:45 AM
sys/dev/vt/colors/vt_termcolors.c
156–159 ↗(On Diff #37172)

Just realized "division to shift" part of the statement above is not entirely correct, it will have to be shift + some rounding logic.

jilles accepted this revision.Dec 30 2017, 10:46 PM
jilles added inline comments.
sys/dev/vt/colors/vt_termcolors.c
156–159 ↗(On Diff #37172)

I agree that this need not be fixed for this commit.

103–104 ↗(On Diff #37198)

This while loop could be omitted, since strtoul() already skips leading whitespace.

This revision is now accepted and ready to land.Dec 30 2017, 10:46 PM
gonzo updated this revision to Diff 37264.Dec 31 2017, 12:00 AM

Remove unneccessary loop, strtoul takes care of leading whitespaces

This revision now requires review to proceed.Dec 31 2017, 12:00 AM
gonzo marked an inline comment as done.Dec 31 2017, 12:02 AM
jilles accepted this revision.Dec 31 2017, 11:26 AM
This revision is now accepted and ready to land.Dec 31 2017, 11:26 AM
This revision was automatically updated to reflect the committed changes.