search

llvm-mos / llvm-mos-sdk

SDK for developing with the llvm-mos compiler
https://www.llvm-mos.org
Other
269 stars 56 forks source link

[Breaking] [supervision] Split sv_lcd_init and sv_lcd_enable. #325

Closed asiekierka closed 6 months ago

asiekierka commented 6 months ago

Technically a breaking change (though nobody uses the target yet), so it would be a good idea to get it in before the next release.

It probably makes more sense this way, but leads to an annoying sv_lcd_init(); / sv_lcd_clear(); / sv_lcd_enable(); chain in main(), if you want to use the LCD, which is a very likely scenario given this is a handheld game console. Maybe it would be a better idea to add an sv_lcd_init(); call to crt0?

mysterymath commented 6 months ago

It probably makes more sense this way, but leads to an annoying sv_lcd_init(); / sv_lcd_clear(); / sv_lcd_enable(); chain in main(), if you want to use the LCD, which is a very likely scenario given this is a handheld game console. Maybe it would be a better idea to add an sv_lcd_init(); call to crt0?

I have controversial opinions about this. I think the "sequential composition operator" (jokey name for the semi-colon) is criminally underused; one should always provide the smallest useful bit of functionality as a separable library call. Otherwise, you end up with printf like interfaces where you wanted a banana, but ended up with the whole jungle.

That being said, there's little harm in giving names to sequential compositions of atomic functions. Maybe something like lcd_setup(args?) would make sense as the first thing to call in main if you wanted the most common configuration? For the same argument above, I'm generally wary of including things in crt0 that aren't intrinsically part of setting up the system, since the first user action may be to undo-them. It looks like there might be different plausible settings for SV_LCD, but I don't have enough knowledge about the Supervision to tell.

asiekierka commented 6 months ago

sv_lcd_init(); is the way it is because there's only one legal configuration: while the SoC supports different LCD panel sizes, you're only really supposed to configure it for the specific LCD used in the Watara Supervision console, that is the 160x160 panel.

I think someone making a new console based on this SoC in 2024 is exceedingly unlikely.

mysterymath commented 6 months ago

sv_lcd_init(); is the way it is because there's only one legal configuration: while the SoC supports different LCD panel sizes, you're only really supposed to configure it for the specific LCD used in the Watara Supervision console, that is the 160x160 panel.

I think someone making a new console based on this SoC in 2024 is exceedingly unlikely.

Ah, it SGTM then to make this part of crt0. If it's the kind of thing where absolutely any plausible project has to do this, then there's no need to make the user type it; that just adds a footgun.

As usual I was a little merge-happy; sorry.