Quote from: names_are_hard on January 31, 2023, 12:03:37 AM
I can give some advice.
Firstly, modules are built outside of the context of any individual cam. This is because modules are supposed to be portable: people can copy them from one card to another in a different cam and they're supposed to work. TCC code on the cam handles loading modules and looking up symbols by name. This is the way that modules find stubs. If a symbol doesn't exist, the user will see an error and the module won't load (but ML won't crash, it's more like a warning than a real error).
If you define something in features.h for a cam, I think it won't be visible to the module during the build. Even if it is, it won't work properly if you copy the module and use it with a different cam, so this isn't a correct approach.
I was suggesting that *things which are functions* could be moved to stubs. I haven't looked at the variables like more_hacks_are_supported, so I don't know what these do or what the best way to handle them is.
You should be able to test your ideas in qemu. First get the existing code working and test it emulates this far (should do I think). Then make changes and see if you broke anything.
Quote from: names_are_hard on January 31, 2023, 04:18:17 AM
I suppose it depends what you mean by "doing stubs wrong"In the module code, where it's just an address, I haven't checked how it's used. In a stub context, it will be treated as a function pointer according to the definition of NSTUB (or ARM32_FN / THUMB_FN if you use those). Is it appropriate to treat these addresses in that way? That depends how the module uses them. In the exact text you gave, you put them in as comments, which will do nothing (and the symbol won't get exported, so the modules won't find it).
Probably, but that's a bigger question than you might realise. I think the intention when ML added lots of supported models was that they'd all get all the features eventually. Later on (I guess) as you've found, FEATURE and CONFIG limit or specify features, but that doesn't work well with modules. Modules are *supposed* to be cam independent, but actually this was never true, there were some edge cases that became apparent when we worked more on modern cams and found that some structs and other internals had changed enough that some assumptions around how modules worked failed.
So, what's an elegant way to let modules behave the right way on a range of cams? My personal feeling is modules could, instead of doing this:Code Select
if (is_camera("5D3", "1.2.3"))
{
screen_x = 0x0101; screen_y = 0x0202;
}
else if { // many other cameras omitted }
Do something like this:Code Select
get_screen_x_y(screen) // screen is a struct with .x and .y members
That is: cleanly separate modules from ML code, so they can only work via exported functions. Modules ask the cam what the right values are for that model. No hard-coding values inside modules.
So in your specific example, create a stub for *all* cams maybe called is_more_hacks_supported() (this is a bad name, but you get the idea). This would probably default to returning false, and be overridden by cams that did have support. Then the module code is greatly simplified - no per cam checks, but something like this:Code Select
more_hacks_supported = is_more_hacks_supported();
Is this the best way to go? That's harder to say. It would increase the size of the ML binary, while reducing the size of modules. Size constraints are a real concern on some cams.
Quote from: names_are_hard on January 31, 2023, 04:18:17 AM
you put them in as comments
didn't want those to compile yet
Quote from: names_are_hard on January 31, 2023, 04:18:17 AM
So in your specific example, create a stub for *all* cams maybe called is_more_hacks_supported() (this is a bad name, but you get the idea). This would probably default to returning false, and be overridden by cams that did have support. Then the module code is greatly simplified - no per cam checks, but something like this:Code Select
more_hacks_supported = is_more_hacks_supported();
is this the correct syntax to do such a thing?
Code Select
NSTUB(1 - is_more_hacks_supported)and cams that do not support it the stub needs to be 0 ? Or you start by saying:
Code Select
more_hacks_supported = 0; //defaults to 0
more_hacks_supported = is_more_hacks_supported() //returns value set in stub
If there is no stub in another cam's Stubs.s does it return 0 automatically?
