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

Wrap ace3ds/r4sdhc.hk in an ifdef and rename r4sdhc_dualcore #118

Merged
merged 7 commits into from
Dec 19, 2017
Merged

Wrap ace3ds/r4sdhc.hk in an ifdef and rename r4sdhc_dualcore #118

merged 7 commits into from
Dec 19, 2017

Conversation

jason0597
Copy link
Contributor

First change is that I wrapped the entire ace3dsplus.cpp and r4sdhc_dualcore.cpp files in #ifdefs.

There is an issue with them not being in an #ifdef.
Suppose that e.g. you had flashcart_core and it was working fine, and then all of a sudden you get commits that support ace3dsplus, which is not yet fully finalized. After the ace3dsplus commits happen, you suddenly get some r4igold3ds fixes or something of the sort, fixes however that are stable and are able to be released.
Now, if you want to release those r4igold3ds fixes (or really any other fix), well, you got to wait until ace3dsplus is fully finished before you mark a release. You can't really grab the latest flashcart_core as it is, with the, still in testing phase, ace3dsplus code and mark a release with that. However, if you could simply have a way to grab the latest flashcart_core, without having those testing phase devices be compiled, you could mark a release.

I made a corresponding pull request to ntrboot_flasher's makefile for the #ifdef changes

==============================================================

Second change that I did is rename the r4sdhc_dualcore.cpp file to r4igoldcc.cpp
The r4sdhc_dualcore.cpp name is not a good name overall, as it causes confusion with the current r4isdhc.cpp.

The reason why I renamed it to r4igoldcc and not say r4isdhc.hk/r4itt/r4ixds is because the r4igold.cc issue is the one that has had the most responses (359!), and seemed to be the main place where the discussion was happening
I also modified the corresponding lines (class declaration, logging, etc.) to match up with the filename change.

I also would like to note that I went ahead and wrote getDescription(). From all of the comments made on issue #3 , and from this comment, I went ahead and wrote that the r4isdhc.hk/r4itt/r4ixds should be compatible as well. This is theoretical and not proven.

@@ -1,3 +1,5 @@
#ifdef TESTING
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this ifdef could be easily moved to ~line 586 (a similar change could be made to r4igoldcc.cpp as well); the "Ace3DSPlus ace3DSplus;" line is what adds it to the list of flashcarts, so it's way overkill to wrap the whole thing in ifdefs. In any case, I hate ifdefs, personally... in most cases anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I concur, we should only be removing that single line.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the difference is that this doesn't even have to compile and definately won't add any bloat to the executable if the whole thing removed, while it still has to compile (which honestly I find a good thing, makes it harder to accidentally make breaking changes) if we only remove that one line.

" * R4i Gold CC (r4igold.cc)\n"
" * R4 SDHC Dual-Core (r4isdhc.hk)\n"
" * R4iTT 3DS (r4itt.net)\n"
" * R4i XDS 2014 (r4ixds.com)";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (r4ixds) actually doesn't appear to be the same cart, on a second look, so please remove it for now :)

@@ -584,3 +586,5 @@ class Ace3DSPlus : Flashcart {

Ace3DSPlus ace3DSplus;
}

#endif
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the ifdef is moved, this line needs to be moved from here to within the curly brace. Again, similar change for r4igoldcc

@kitlith
Copy link
Member

kitlith commented Dec 17, 2017

I agree with the rename of r4isdhc_dualcore to r4igoldcc. I agree that the old name was bad, it came from when I first looked at the flasher for a r4isdhc.hk cart, but later we supported the r4isdhc.com carts...

also removed the r4i xds from the r4igoldcc description
@jason0597
Copy link
Contributor Author

commited changes

@angelsl
Copy link
Member

angelsl commented Dec 17, 2017

I think the title should be "r4igold.cc" or just "R4i Gold". I'm inclined to the former, then we can rename R4iSDHC to r4i-sdhc.com or something like that, since all these carts don't really have a unique identifying name aside from the domain name, unlike Ace3DS or Acekard 2i.

@jason0597
Copy link
Contributor Author

jason0597 commented Dec 17, 2017

@angelsl there are so many ifs and buts everywhere that you have to explain a lot in the description
all the clones, the exceptions, hw revisions, etc

e.g. the r4isdhc.com dual-core, the r4isdhc.hk dual-core, and the r4isdhc.com.cn dual-core are VERY different things. you can't really say all that in just the title

also, you can't put dots in the class name. though, it probably isn't that big of a deal since the developers can figure out which class belongs to which family of flashcarts

@angelsl
Copy link
Member

angelsl commented Dec 17, 2017

Yes, I said "r4i-sdhc.com" specifically — note the TLD

@kitlith
Copy link
Member

kitlith commented Dec 18, 2017

LGTM, I'll probably merge tonight after adding a commit to change the titles a bit.

Title of the cart is now more accurate.
@angelsl says that this is basically ready to go. :D
@kitlith kitlith dismissed pixel-stuck’s stale review December 19, 2017 08:05

The changes have been made, it's good to go now.

@kitlith kitlith merged commit 339878f into ntrteam:master Dec 19, 2017
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.

5 participants