Skip to content
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

dmnt.gen2: More logging in GDB #2288

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MonsterDruide1
Copy link

There are a lot of things that can go wrong when dealing with GDB, and most of them are caught by Atmosphere in a few checks before actually running "dangerous" stuff. However, when actually running into an issue, Atmosphere currently just sends back an error message without further explanation. As the GDB protocol doesn't intend to include error messages, these should at least be logged to the logfile, if logging to a file is enabled.

This PR goes through the whole GDB implementation, and attaches a message to every E01 response, clarifying the cause of the issue (as far as it's known). This will help in debugging further issues with GDB and its protocol.

Side note: This PR contains quite simple changes, so I can get to know the process of contributing to Atmosphere. I did already implement some changes to make the multiprocess+ optional for working with older/simpler GDB clients, which I will clean up and PR once this is in.

@SciresM
Copy link
Collaborator

SciresM commented Mar 12, 2024

Well, I am on vacation in Japan until March 25th.

That aside, step 1 for contributing to atmosphere is "reach out to me on discord" to talk about whatever you're looking to contribute. I generally don't accept unsolicited pull requests...

...in this case, I don't think that logging outside of debug configurations is actually desirable.

I think I would want this ifdef'd to continue being a null-operation under default build config.

Anyway the actual commentary is that you don't seem to understand atmosphere's coding style - you have a variable namedLikeThis, but atmosphere exclusively uses named_like_this for local variables.

@MonsterDruide1
Copy link
Author

Alright, I didn't find any contribution or styling guide regarding this project, so I expected getting something wrong - thanks for clarifying.

I can't contact you/write my intentions on the ReSwitched Discord server, because I'm blocked from writing in any major channels - only #dev-support would be open, but doesn't quite match the topic, as I don't need help with it. Should I DM you instead?

This logging is not done "normally", only if the debugging for GDB is explicitly enabled.

Also, I adjusted the name to should_log_result - thanks for the comment.

@SciresM
Copy link
Collaborator

SciresM commented Mar 12, 2024

Yes, #dev-support would be the correct channel, though usually I just have people DM me.

Actually reviewing the logs...I am not really sure I want these in? A lot of them are clarifying cases that should never occur using devkitPro's gdb, which is the only guaranteed-supported client to the stub.

Also, why log reply/response - isn't that the purpose of gdb's "set debug remote 1"?

@MonsterDruide1
Copy link
Author

Alright, let's continue this in DMs then, if that's what you prefer!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants