hal: Mark outdated functions as deprecated and remove unused font #458
Conversation
lib/hal/debug.h
Outdated
| @@ -23,13 +23,21 @@ extern "C" | |||
| * Prints a message to whatever debug facilities might | |||
| * be available. | |||
| */ | |||
| __attribute__((deprecated("deprecated as of 05/2021", "Use SDL2 for generic text output"))) | |||
There was a problem hiding this comment.
The SDL2 message is a bit vague / misleading. There are 2 primary uses for hal/debug.h:
- Debugging: also used within nxdk, no replacement exists. SDL2 is not suitable here.
- Dumb UI: also used in nxdk samples. Replacements do exist, in the form of other rendering libraries. SDL2 is suitable, but also typically overkill.
Therefore, I'd probably not deprecate debugPrint just yet.
Rest should be deprecated though.
I believe there are also variables in hal/debug.c that people still like to use (to move the cursor around). That has always been a hack though.
There was a problem hiding this comment.
So instead of just using debugPrint i'll have to initiate an entire SDL instance (wasting memory) just to display text on the screen? Not just SDL, I'd have to init SDL_TTF which is more overhead, and through experiments I've done TTF is quite slow at drawing text to a surface, so much so even the documents for it talk about better ways to write to the screen with TTF which involves pre-rendering all glyphs (See for example: https://github.com/GXTX/BadApple_SDL/blob/ee51c27f769c5432c30ece7dcaab6aae1649972a/main.c#L52).
I don't like this. What's the replacement for debugPrint.
There was a problem hiding this comment.
an entire SDL instance (wasting memory)
[...]
Not just SDL, I'd have to init SDL_TTF which is more overhead,
You have a fixed 64MiB anyway and it's not like you are stealing it from another process.
Chances are you won't be hitting memory bounds when doing a console application.
I'd be more concerned with amount of boilerplate code.
TTF is quite slow at drawing text to a surface
Unless you are making an ASCII 3D renderer (but even then the hal/debug stuff is not suitable for you) your text rendering performance won't matter - no user can read as fast as you dump out pixels.
I'm probably the worst offender when it comes to debug message spam during game init on neverball and SWE1R; but I can also disable those prints if they are too slow.
I don't like this. What's the replacement for
debugPrint.
I'd prefer to remove it sooner than later.
So even if it temporarily stays (and isn't "officially" deprecated yet), consider it a stupid move to use it.
Nobody prevents you from taking hal/debug.c into your own project, it's a single file to copy.
So the maintenance overhead, unclean design and license shenanigans are not worth to keep it long-term.
Personally, I'd recommend to make your own lib with an interface like this:
typedef struct {
struct {
void* address;
unsigned int width,
unsigned int height,
unsigned int stride,
SDL_PixelFormatEnum format;
} buffer;
struct {
int x;
int y;
} cursor;
struct {
const void* address;
unsigned int width;
unsigned int height;
unsigned int stride;
SDL_PixelFormatEnum format;
} font;
} SDL_FB_LOG;
#define SDL_FB_LOG_DEFAULTS {...}
void* sdl_fb_log_allocate(SDL_FB* fb);
sdl_fb_log_printf(SDL_FB* fb, const char* fmt, ...);..so basically a glorified memcpy to move stuff around. It will be more stable than hal/debug, more flexible, potentially faster etc.
Most importantly: You can blit the resulting framebuffer with SDL2, XGU or PVIDEO, and show and hide the terminal whenever you want (unlike hal/debug which overwrites your memory which quickly corrupts everything).
A custom lib can also be platform independent, so you can avoid adding it to nxdk, which should always be preferred for random libs that don't need explicit Xbox support.
The only thing that would require Xbox support for a lib like this would probably be cache flushing, but I assume debug.h also won't do it properly. If you present through SDL2 or similar, then you don't have to worry about it either.
Also writing these functions will take like.. 30 minutes? Bonus points for adding some sort of color mapping.
You could also steal similar libs from some other platforms standard library (like PSP, Gamecube, 3DS, ...).
I assume the samples will eventually use an SDL2 UI framework of some sort.
There was a problem hiding this comment.
Personally, I'm very torn on this one. On one hand, it isn't good to have to carry this code around, and we risk people relying on it for too much, but on the other hand, it is the most used debugging tool for me, and probably others as well.
I read something a while back that described how on the PS3, stdout and stderr are essentially tunneled over the network, and output on the developer's system. This is something I had in mind for nxdm (another thing I can't seem to find the time for), along with intercepting DbgPrint and friends. This would at least reduce the need for debugPrint (together with providing a gdb stub), but even then, I see myself needing the opportunity to get something output without adding a whole pile of complexity quite often.
Are there any standard solutions we could add instead? I would like to not recommend users to roll their own, I suspect it would just lead to people copying old versions of debugPrint from project to project. Some lib maybe that gives you relatively simple text output capabilities, but can be pointed to an arbitrary framebuffer (so it could be adapted to SDL or PVIDEO trivially)? Not to mention that we currently also have pb_print...
There was a problem hiding this comment.
pb_printis an absolute nightmare to me; it wastes RAM, is very slow and works against the architecture (it does per-"pixel" drawing on the GPU, so it sends thousands of individual commands). Overall, I hope that pbkit will get major refactors (slimming down) in the future. It's easy enough to set up PVIDEO (needs a lib / code) or to send a draw call for a surface which contains the framebuffer graphic.- I'd like to see nxdk split into the kernel-space section (CXX / C support + low-level drivers) and the user-space facing APIs (winapi, SDL, ...). kernel-space should never rely on anything in user-space.
- I'd like to not rely on
SDLorwinapiin kernel-space, so printing with those isn't an option. - I don't want large font graphics as part of the kernel-space section, so it can't do visual drawing or it must communicate to optional parts (→
DbgPrint,std::cerr,fprintf(stderr, ...)are probably good standard APIs). - There should probably be:
- Optional
libnxdk_print_screen.libwhich catches some of that and renders them to some framebuffer - Optional
libnxdk_print_usb.libwhich would send to CDC after USB: Replace OHCI USB host library #438 - Optional
libnxdk_print_network.libwhich sends data via network somehow - These don't have to be compatible with each other (in my opinion), but if you provide none, the app should still work.
- Optional
- That
libnxdk_print_screen.libwould use another independent, simple, drawing lib +DbgPrinthooking lib, so it's also possible for people to diverge fromlibnxdk_print_screen.libwithout writing a lot of code. - An additional
libnxdk_initialize_screen.libcould be used to also set up XVideo, for the laziest ports (similar to theautomount_d). Standard C console apps should work out of the box then. debugPrintshould probably be replaced byDbgPrint(kernel-space like-code) orOutputDebugString(samples which are equivalent to user-space level); in some instancesSDL_Logmight also be an option, which can be re-routed toOutputDebugString.printfshould probably be routed toDbgPrint?- The use of internal
DbgPrintshould be#ifguarded, so we can compile it away for performance builds / avoid spamming the users production build about internal events. Spurious printing (= something from nxdk prints, even if you didn't request it) has always been an annoying issue with nxdk for me; with debug.h it can easily corrupt your application memory, but it's also annoying in general.
There was a problem hiding this comment.
Pb_Print is definitely something I would agree with being a nightmare, especially on a lower end console, I could see it not being a big deal if it was being utilized on a newer console generation, however this is just personal opinion, not to be taken as logistical advice
Edit: Would it be of any consequence to implement something like ImGui, as I've saw it being ported to systems of lower power. Would be a nice GUI to work with
2nd Edit: here is something I'm adding here for reference https://github.com/Rinnegatamante/imgui-vita this remark is virtually inconsequential I know, but the vita is slightly more powerful than the xbox with in theory a 10 gigaflop difference, in practice the disparity is probably greater, I'm just throwing out ideas.
final edit (potential nit: for you jfr so I’m addressing it here): I’m out of town so I’m using the app and I don’t think this ended up exactly where I meant for it to, but I believe that it can still be found appropriate to the topic.
Dumb UI: also used in nxdk samples. Replacements do exist, in the form of other rendering libraries. SDL2 is suitable, but also typically overkill.
this is what I was referring to
There was a problem hiding this comment.
@MasonT8198 there are enough options for more complicated UIs. This discussion focuses on debugPrint which is often used for CLI-like applications and simple tools (like reporting status of an EEPROM dumper, which doesn't need any UI, or a unit-test or minimalistic sample for an API).
Other things XboxDev people got involved with:
- https://github.com/Tyyppi77/imgui_sdl
- https://github.com/Ryzee119/lvgl-sdl
- (with XGU / pbkit there are many more powerful options which are easy to port, as it usually comes down to 1-2 shaders and a handful of drawing functions, but if you use XGU / pbkit, you probably bring your own)
However, the only case that imgui and other high level libraries cover, is actual applications, designed for Xbox or framebuffer based systems.
But they all need additional code and don't support:
#include <stdio.h>
int main() {
printf("Hello world from a tiny sample!");
}or:
void functionInSomeLibOrLowLevelDriver() {
#if DEBUG_BUILD
debugPrint("Some note about stuff that happened near the kernel");
#endif
}So it doesn't support libraries / core functionality which don't have control over UI, and it also doesn't cover the case for "very-little-code" project.
The last point is critical for nxdk adoption, because you can't expect some beginners to write a fully fledged UI (not even with toolkits like imgui).
Even experienced developers like thrimbor, ryzee, or I will want a simple option for quick PoCs. Most of my small projects eventually went into xbox-tools / xboxpy, because I couldn't even be bothered to write C boilerplate or to transfer my code manually - my threshold for "too much boilerplate work" is that low.
There was a problem hiding this comment.
Thank you for the Clarification and Input, I rather appreciate it (I’m rather new to the scene so getting input and stuff is helpful for me learning all this)
This discussion focuses on debugPrint which is often used for CLI-like applications and simple tools (like reporting status of an EEPROM dumper, which doesn't need any UI, or a unit-test or minimalistic sample for an API).
I do agree that passing debugPrint as an argument called dbgPrint or XdbgPrint (This one is just attributing to the X theme) would be better as it is rather more associated to kernel-level abbreviation styles.
But they all need additional code and don't support:
#include <stdio.h>
int main() {
printf("Hello world from a tiny sample!");
}
or:
void functionInSomeLibOrLowLevelDriver() {
#if DEBUG_BUILD
debugPrint("Some note about stuff that happened near the kernel");
#endif
}
I had overlooked this in the moment of writing the previous statement, I apologize. I do agree SDL2 UI is rather overkill for something like this.
There was a problem hiding this comment.
More clarification, so it's not just me and thrimbor who are on the same page:
DbgPrintis an existing export from the Xbox Kernel, and triggers an interrupt that's meant for the debug service (so a kernel debugger would catch and display it) (fmt string supported).
(MSDN)OutputDebugStringis an existing winapi function which is basically doing the same asDbgPrint(no fmt string supported).
(MSDN)- For higher level code,
SDL_Logis also a redirectable logging function, and then there's stdio/iostream which might also be suitable, especially if we just rereoute them to the lower-level functions by default.
There's an existing proof of concept by thrimbor, which registers a handler for the debug service / interrupt (also the XDK would do it): #172 (comment)
Because those APIs have to exist anyway (also printf and other C friends), it makes sense to use them (although it's slightly more limited than the existing visual printing, which also supports moving the cursor etc - as done in older versions of https://github.com/Ryzee119/Xenium-Tools).
lib/hal/debug.h
Outdated
| @@ -23,13 +23,21 @@ extern "C" | |||
| * Prints a message to whatever debug facilities might | |||
| * be available. | |||
| */ | |||
| __attribute__((deprecated("deprecated as of 05/2021", "Use SDL2 for generic text output"))) | |||
There was a problem hiding this comment.
Surprised you can have comma separated messages? GNU gcc man-pages say:
deprecated
deprecated (msg)
Also not sure if the message should contain the date like that.
(Edit: I see that you linked the clang docs; however, they make it seem like replacement is actually meant for automated fixes? Not sure)
There was a problem hiding this comment.
Unresolved, because the question about clang docs remain: is replacement meant as an auto-replacement hint that must be machine readable?
lib/hal/audio.h
Outdated
| __attribute__((deprecated("deprecated as of 05/2021", "Use SDL2 for audio output"))) | ||
| void XAudioInit(int sampleSizeInBits, int numChannels, XAudioCallback callback, void *data); | ||
| __attribute__((deprecated)) | ||
| void XAudioPlay(); | ||
| __attribute__((deprecated)) | ||
| void XAudioPause(); | ||
| __attribute__((deprecated)) | ||
| void XAudioProvideSamples(unsigned char *buffer, unsigned short bufferLength, int isFinal); |
There was a problem hiding this comment.
I don't think these functions are actually deprecated - I wouldn't recommend people use it, because it's a low-level interface, but it's not like SDL replaces it, it just puts a sane abstraction over it.
There was a problem hiding this comment.
(I'm sure I already left a comment about this somewhere, but I can't seem to find it anymore?)
Yes, the API is deprecated, but the implementation isn't.
It depends on how you look at the deprecation attribute.
I think there should be a guard in the beginning, like this:
(Macro name and message should probably be refined)
#ifndef INCLUDED_FROM_SDL2
#warning XAudio API is deprecated. Use SDL2 audio instead
#endif(same thing could also be done for deprecated function attributes, not sure what's better / worse)
There was a problem hiding this comment.
@thrimbor Was this a nit, or do you expect changes? If so, how could this be implemented?
Could this potentially trigger warnings when SDL is included?
I'd like to avoid tripping compilation of libs because of -Werror -Wdeprecated
There was a problem hiding this comment.
Oops, managed to miss this comment.
I think it's best to remove the deprecation attribute for now, and just add a comment in the audio header stating that this is only supposed to be used as a backend and that something like SDL2 should be preferred for applications.
lib/hal/debug.h
Outdated
| @@ -23,13 +23,21 @@ extern "C" | |||
| * Prints a message to whatever debug facilities might | |||
| * be available. | |||
| */ | |||
| __attribute__((deprecated("deprecated as of 05/2021", "Use SDL2 for generic text output"))) | |||
There was a problem hiding this comment.
Personally, I'm very torn on this one. On one hand, it isn't good to have to carry this code around, and we risk people relying on it for too much, but on the other hand, it is the most used debugging tool for me, and probably others as well.
I read something a while back that described how on the PS3, stdout and stderr are essentially tunneled over the network, and output on the developer's system. This is something I had in mind for nxdm (another thing I can't seem to find the time for), along with intercepting DbgPrint and friends. This would at least reduce the need for debugPrint (together with providing a gdb stub), but even then, I see myself needing the opportunity to get something output without adding a whole pile of complexity quite often.
Are there any standard solutions we could add instead? I would like to not recommend users to roll their own, I suspect it would just lead to people copying old versions of debugPrint from project to project. Some lib maybe that gives you relatively simple text output capabilities, but can be pointed to an arbitrary framebuffer (so it could be adapted to SDL or PVIDEO trivially)? Not to mention that we currently also have pb_print...
|
This PR can probably not advance any further without an agreement on the replacement for
I added that date with the thought that I would enable people to not only search for a fix(when encountering that message) at the place of github/nxdk but also in time(git history etc.). 🤷♀️ Maybe I over thought it a little. Anyway, we need an agreement on that message as well. I resolved some conversations, against the usual workflow, prematurely as I found the topic of discussion mostly unrelated to this PR and the discussion should have been moved to the referenced issue instead(imo). So to keep things clear and productive I resolved them. My second(and maybe following) commit will be squashed together with the first one when the time is right, for now they will stay separated to keep clean about what is decided and implemented - and what is still on debate. |
I unresolved the
I also unresolved some others and left comments as to why I consider them unresolved.
My opinion, to avoid any bikeshed: Remove deprecation of Because it's so heated, also try to start a new discussion how it could be deprecated (= create an issue about a new visual printing API for console like applications; which seems to be the main point that is not covered by #172).
I'd remove it for consistency with how we deprecated things in the past. In fact, in the past we didn't even add messages: 8e5a12f, so I suggest to also remove message strings and the Most replacements are also common-sense - the reason we deprecate is to give people time to avoid bad code, not necessarily to teach them how to resolve it. We should not over-engineer this. A stable solution (like solution we had in the past) is preferred over using shiny new toolchain features (like deprecation |
|
I would like to but I still wait for your feedback here. |
Teufelchen1
changed the title
|
Sorry it took me so long 📟. Any other issues with this PR? |
lib/hal/audio.h
Outdated
| // note that I currently ignore sampleSizeInBits and numChannels. They | ||
| // The XAudio API is only supposed to be used as a backend. Using SDL2 for | ||
| // audio playback should be prefered for applications. | ||
| // note that currently sampleSizeInBits and numChannels are ignored. They |
There was a problem hiding this comment.
nit: "They" at end of line is equally unfortunate (I did not check how the comment continues, but this looks odd).
Also not sure if we do this in nxdk, but I personally also usually use markdown for code stuff in comments, so I'd use:
[..] currently `sampleSizeInBits` and [..]
There was a problem hiding this comment.
Applied both nits and the markdown-code-comment idea.
Hi 🙋♀️
Following JFRs analysis in issue #449, I marked a bunch of functions as deprecated.
I also added messages to commonly used function on how to replace them.
As I wasn't sure how to deprecate a font, I just added the deprecated attribute to the top of the file
font_sasteroids2.hConfusingly, JFRs analysis included functions that have already been removed in the past. I took no further actions regarding them.
After this PR(or in this PR), some samples need to be adjusted as they use now deprecated functions 😅
Edit:
For the interested reader, the clang deprecated attribute is documented here.