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

default_arm7: build options to include dswifi, maxmod, libxm7 #248

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

GalaxyShard
Copy link
Contributor

Decided to take a stab at #242. I haven't tested this code much besides compiling, so it may not be quite ready to merge.

This changes the output path of the binary from "arm7.elf" to "$(BUILDDIR)/arm7.elf", which might have to be accounted for elsewhere. I'm not sure how to go about changing the default Makefiles for this, since it currently leaves no options to disable maxmod or dswifi (should there be more Makefile templates? options within the current templates?)

I wasn't entirely sure what FIFO to use for libxm so I copied the one from the examples (user 7, ie channel 14) which is less than ideal. Assuming maxmod and libxm7 cannot be mixed together, maybe the FIFO reserved for maxmod can be used (channel 3)?

In the Makefile, the DEFINES were originally only being passed to LDFLAGS and not CFLAGS/CXXFLAGS/..., I think that was a bug so I added them to the other flags as well (although no defines were used before so it didn't make a difference)

Copy link
Member

@AntonioND AntonioND left a comment

Choose a reason for hiding this comment

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

Changing the path of the default ARM7 is going to cause a lot of people to complain. This PR would need to preserve the path and add the other elf files to the same folder.

Regarding the FIFO channel for LibXM7, I think it's fine this way. If someone doesn't like it, they can have a custom ARM7 core and change it.

}

int main(int argc, char *argv[])
Copy link
Member

Choose a reason for hiding this comment

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

Also, I would leave this the way it was before, even if it's unused. It may be useful some day, and it's better to leave the arguments there because people tend to copy the example code right away and forget about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-int main(void)
+int main(int argc, char *argv[])
 {
+    // Unused variables
+    (void)argc;
+    (void)argv;
+    
     // Initialize sound hardware
-int main(void)
+int main(int argc, char *argv[])
 {
     // Initialize sound hardware

Which one of these would be preferred? The first one avoids unused variable warnings.

Copy link
Member

Choose a reason for hiding this comment

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

If you want to silence the warnings, go ahead (even though by default in the Makefile they aren't enabled, so they probably only appear in your setup).

Copy link
Contributor

Choose a reason for hiding this comment

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

argc/argv is not implemented and will probably not be implemented on ARM7, so providing it in the main() function signature feels a little deceptive/misleading.

Copy link
Member

Choose a reason for hiding this comment

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

Well, fair enough, we can leave the arguments out. We can change it back if they are used for something at some point.

@GalaxyShard
Copy link
Contributor Author

Changing the path of the default ARM7 is going to cause a lot of people to complain. This PR would need to preserve the path and add the other elf files to the same folder.

How should they be named? I assume the maxmod+dswifi combo should stay arm7.elf, should it also have a copy with a more descriptive name like "arm7_wifi_maxmod.elf", then for the others "arm7_maxmod.elf", "arm7_wifi.elf", "arm7_wifi_libxm7.elf", etc?

Regarding the FIFO channel for LibXM7, I think it's fine this way. If someone doesn't like it, they can have a custom ARM7 core and change it.

I think it might be surprising to newcomers for a user channel to be used for audio in the builtin arm7. If I'm not mistaken the maxmod channel would be unused when libxm7 is in use, so that one could be used (unless we want to support both libraries in the same binary?); perhaps it would need a rename to "FIFO_MAXMOD_LIBXM7" with a alias of "FIFO_MAXMOD" for compatibility, similar to the FIFO_STORAGE/FIFO_SDMMC.

@AntonioND
Copy link
Member

AntonioND commented Dec 28, 2024

How should they be named? I assume the maxmod+dswifi combo should stay arm7.elf, should it also have a copy with a more descriptive name like "arm7_wifi_maxmod.elf", then for the others "arm7_maxmod.elf", "arm7_wifi.elf", "arm7_wifi_libxm7.elf", etc?

Well, there are a few options:

  1. We can have multiple folders: maxmod_dswifi, libxm7_dswifi and one arm7.elf inside. Then, have a copy of arm7.elf in the old location.
  2. We can have maxmod_dswifi.elf, libxm7_dswifi.elf, etc, and then have the makefile copy maxmod_dswifi.elf to create arm7.elf.
  3. Something else?

I think it might be surprising to newcomers for a user channel to be used for audio in the builtin arm7. If I'm not mistaken the maxmod channel would be unused when libxm7 is in use, so that one could be used (unless we want to support both libraries in the same binary?); perhaps it would need a rename to "FIFO_MAXMOD_LIBXM7" with a alias of "FIFO_MAXMOD" for compatibility, similar to the FIFO_STORAGE/FIFO_SDMMC.

The problem was reserving one channel for Maxmod, not the fact that LibXM7 is using an user channel. If anything, we should add a new alias such as ´FIFO_MUSIC_PLAYERforFIFO_MAXMOD`. However, I'm still not very convinced this is a great idea. What if someone wants to have two audio libraries for some reason? Asking users to decide which FIFO channel to use is better than hardcoding one that the library then expects you to use.

@GalaxyShard
Copy link
Contributor Author

1. We can have multiple folders: `maxmod_dswifi`, `libxm7_dswifi` and one arm7.elf inside. Then, have a copy of `arm7.elf` in the old location.

2. We can have `maxmod_dswifi.elf`, `libxm7_dswifi.elf`, etc, and then have the makefile copy `maxmod_dswifi.elf` to create `arm7.elf`.

3. Something else?

I like keeping build artifacts under build/ and then copying the maxmod+dswifi into default_arm7/arm7.elf for compatibility, although option 2 would be more in-line with what the rest of BlocksDS does so I'll go with that for now (the crts are built in a very similar way to option 2).

The problem was reserving one channel for Maxmod, not the fact that LibXM7 is using an user channel. If anything, we should add a new alias such as ´FIFO_MUSIC_PLAYERforFIFO_MAXMOD`. However, I'm still not very convinced this is a great idea. What if someone wants to have two audio libraries for some reason? Asking users to decide which FIFO channel to use is better than hardcoding one that the library then expects you to use.

Realizing now that the user channels would only be used if the arm7 was overridden anyways, so it would make sense to use a configurable user channel for libxm7. Does maxmod hardcode channel 3? If not, maybe FIFO_MAXMOD could just be made into an extra user channel and be used in the default_arm7 for maxmod so that it can be overridden like libxm7 (I guess this would be a separate issue though).

@AntonioND
Copy link
Member

AntonioND commented Dec 29, 2024

I like keeping build artifacts under build/ and then copying the maxmod+dswifi into default_arm7/arm7.elf for compatibility, although option 2 would be more in-line with what the rest of BlocksDS does so I'll go with that for now (the crts are built in a very similar way to option 2).

I think that the main problem here is that default_arm7 is not a good name for what you're trying to do, anyway. Let's do something else. We can have the old default_arm7 renamed to just arm7. Then, inside that folder, we can have several folders:

arm7
|- maxmod
|  |- arm7.elf
|- maxmod_dswifi
|- maxmod_dswifi

Now the problem is where to keep the main.c file. This pattern would make it easier to add random ARM7 cores later: we just add another folder with code that isn't related to any of the other cores.

In order to keep compatibility with the old path, we can have sys/Makefile copy the Maxmod+DSWiFi elf file to sys/default_arm7/arm7.elf.

If we had this:

arm7
|- maxmod.elf
|- maxmod_dswifi.elf
|- maxmod_dswifi.elf

We would have to copy files with a Makefile to the parent folder of the Makefile, which isn't very logical.

Realizing now that the user channels would only be used if the arm7 was overridden anyways, so it would make sense to use a configurable user channel for libxm7. Does maxmod hardcode channel 3? If not, maybe FIFO_MAXMOD could just be made into an extra user channel and be used in the default_arm7 for maxmod so that it can be overridden like libxm7 (I guess this would be a separate issue though).

It is hardcoded, yeah: https://github.com/blocksds/maxmod/blob/852f36750c733000a55693dff0b467e4a6e2658e/source_ds9/mm_main9.s#L19

Hardcoding this in libnds and maxmod was a bad choice, I agree.

@GalaxyShard
Copy link
Contributor Author

Now the problem is where to keep the main.c file. This pattern would make it easier to add random ARM7 cores later: we just add another folder with code that isn't related to any of the other cores.

What if it is structured like this:

arm7/
|-  Makefile
|
|-  main_core/
|   |- arm7_dswifi_libxm7.elf
|   |- arm7_dswifi_maxmod.elf
|   |- arm7_dswifi.elf
|   |- arm7_libxm7.elf
|   |- arm7_maxmod.elf
|   |- arm7_minimal.elf
|   |- build/...
|   |- main.c
|   |- Makefile
|   |- Makefile.generic
|
|-  some_other_core/
|   |- arm7.elf
|   |- build/...
|   |- main.c
|   |- Makefile
|
default_arm7/
|-  arm7.elf    (copied here from ../main_core/arm7_dswifi_maxmod.elf, by ../Makefile)

This would be flexible enough to add new cores in the future and allows the cores to be configurable, without duplicating main.c

@AntonioND
Copy link
Member

AntonioND commented Dec 30, 2024

Yeah, I think that works for me!

@GalaxyShard
Copy link
Contributor Author

I tested this and it seems to work with the libxm7 examples after replacing the custom arm7 with the main_core arm7_libxm7.elf. This does leave the question of how the different cores would be selected though, since the rom_arm9 always uses the dswifi+maxmod arm7. To test the libxm7 examples I threw together a patch for rom_arm9/Makefile that makes the arm7 path configurable relative to sys/arm7/ although I'm not sure if that's the way to go. (GalaxyShard@99236df)

Copy link
Member

@AntonioND AntonioND left a comment

Choose a reason for hiding this comment

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

I like how this looks now, fix the few issues I've mentioned and this is good to go.

sys/Makefile Outdated Show resolved Hide resolved
sys/arm7/main_core/Makefile Outdated Show resolved Hide resolved
install: all
@echo " INSTALL $(INSTALLDIR_ABS)"
@test $(INSTALLDIR_ABS)
$(V)$(RM) $(INSTALLDIR_ABS)
Copy link
Member

Choose a reason for hiding this comment

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

You need -rf here: $(V)$(RM) -rf $(INSTALLDIR_ABS)

Copy link
Contributor Author

@GalaxyShard GalaxyShard Jan 1, 2025

Choose a reason for hiding this comment

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

I believe $(RM) expands to rm -rf throughout all the Makefiles that define it

sys/arm7/main_core/Makefile Show resolved Hide resolved
@AntonioND
Copy link
Member

AntonioND commented Dec 31, 2024

I tested this and it seems to work with the libxm7 examples after replacing the custom arm7 with the main_core arm7_libxm7.elf. This does leave the question of how the different cores would be selected though, since the rom_arm9 always uses the dswifi+maxmod arm7. To test the libxm7 examples I threw together a patch for rom_arm9/Makefile that makes the arm7 path configurable relative to sys/arm7/ although I'm not sure if that's the way to go. (GalaxyShard@99236df)

I think we just have to document how to use this. We could make a change to all makefiles and add an ARM7ELF variable to the top, or we could tell people to go to the ndstool command and edit it there. In one of my private repositories I've been using that ARM7ELF name and it works well:

ARM7ELF	:= $(BLOCKSDS)/sys/arm7/...

...

$(ROM): $(ELF)
	@echo "  NDSTOOL $@"
	$(V)$(BLOCKSDS)/tools/ndstool/ndstool -c $@ \
		-7 $(ARM7ELF) -9 $(ELF) \
		-b $(GAME_ICON) "$(GAME_FULL_TITLE)" \
		$(NDSTOOL_ARGS)

This way, old makefiles will still work, and people can choose to update them or not.

sys/arm7/main_core/source/main.c Show resolved Hide resolved
sys/arm7/main_core/source/main.c Outdated Show resolved Hide resolved
@@ -197,7 +199,7 @@ endif
$(ROM): $(ELF)
@echo " NDSTOOL $@"
$(V)$(BLOCKSDS)/tools/ndstool/ndstool -c $@ \
-7 $(BLOCKSDS)/sys/default_arm7/arm7.elf -9 $(ELF) \
-7 $(ARM7ELF) -9 $(ELF) \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are these templates supposed to include a copy of all the default_makefile code or is this supposed to look more like the examples/ where there is a include for the boilerplate?

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.

3 participants