Removing global context around pm3 connectivity (work in progress)

[micolous]

This is related to cleanup work in #334. This includes PR #341, so should wait until that is merged. It is quite large, and not finished or tested as much as I'd like yet, but will give you an early preview.

It:

• Refactors a bunch of global state so that it is stored in a new pm3_connection structure.
• Refactors PM3 communications into a new comms.c module.
• The flasher now uses the same code paths for communicating with the PM3 as the main client.

Caveats:

• The client does not make use of the ability to start up multiple connections.
• These changes do not permit the Lua bindings to use multiple connections. This stuffs a reference to the pm3_connection structure inside Lua's global state, and then all the methods just read off that.
• There's a bunch of buffers that I've stashed in the pm3_connection struct for lack of a better place.
• save_restore{DB,GB} have a "global" stash which all connections can access.

TODO:

• Get the GUI working.
• CMD_DOWNLOADED_RAW_ADC_SAMPLES_125K and the mechanisms around it need improvement -- it is possible to use it overwrite arbitrary memory within the memory space of the client. I'm still working out a good way to handle this without breaking too much stuff, because as far as I can tell, it's intended as a one way DMA.
• Cleanup the startup process, so that it can be shared between the flasher and client, and other things could embed it without writing a bunch of boilerplate.
• Make the debug level selection global again.
• Fix the flasher.
• Write some tests.
• Do some more testing.
• Rebase and squash commits down.

[iceman1001]

Impressive,
Do please explain what is the gain of having the new pm3connection structure everywhere?
Is the idea that the pm3 client should connect to several devices in the future? As it looks now its shoving around the conn structure all over the code just to remove a few global vars? Well, not true since I saw you added the usbcommand array into aswell now. Meaning a connection have its own commands, and demodbuffer?!.
ok, I can see some potential here, having multiple devices and control them with one client. Not sure when thats applicable as a real life scenario.

btw, sidenote
the debug part is set during execution "data setdebug", so having it set during compilation is a step down in functionality.

[micolous]

Do please explain what is the gain of having the new pm3connection structure everywhere?

This removes many global variables from the client. This would allow:

• establishing connections with multiple PM3s in a single binary.
• easily handling disconnections and reconnections without requiring restarting the binary (which matters most on the "library" side of things) -- I can just destroy a pm3_connection struct and create a new one.
• a library to be created that allows communication with a pm3, so scripting can be done with "pm3 embedded in some other process" rather than the other way around (as it is with Lua).

And yes, this pretty much stashes everything that looks like a global variable into that connection struct. Some things like the command buffer I want to pull out of there and stash it somewhere else, but this is the simplest interface without significantly rewriting the pm3 client.

the debug part is set during execution "data setdebug", so having it set during compilation is a step down in functionality.

I'll put that on the TODO list, but as that code is sitting in common, it would end up with a different calling convention in order to get access to debug level information (currently stashed in that connection object). I'm thinking it may have been better to leave g_debugLevel as global variable, there's not a lot of harm in having that shared between multiple instances.

[iceman1001]

Thanks for the answers, would you mind telling more about destroy and create the pm3_connection struct and how it will enable restarting connection without restarting binary. I'm very curious about this part for obvious reasons :)

The debug is twofolded, on clientside its mostly used for LF code. the g_debuglevel has nothing todo on the deviceside. The deviceside has more then one.. Since you are upto changing it, what would your suggestion be for that? A unified debug level on both client/device side or two seperate ones?

[pwpiwi]

Besides getting rid of global state and encapsulating communication in a separate module, you have also added the long overdue recv_lock. 👍

Re g_debugmode I suggest to make a separate module for debugging functions and use set_debugmode() and get_debugmode() functions ("methods") instead of accessing g_debugmode directly.

[pwpiwi]

Please don't try to put everything in one PR. You are risking that it will never be accepted because no one can thoroughly test it. You could start with putting all comm functions in comms.c and comms.h but still have global comms state. Then deglobalize comms struct. Then the debugging.

[micolous]

Re: g_debugLevel de-globalization, I've now excluded that from this PR. It now lives in ui.h, and is now only declared once (previously it was declared in two different header files). I've noticed a pattern with the code that function headers are declared in many different places in the client.

I still need to shuffle it in the armsrc code, so that it has a place where it is declared once as well (right now it's a define as a holdover from a previous version of the change.

Regarding destroying and creating the pm3_connection struct, I haven't written this bit properly yet, so it's gnarly and declared in two places. Part of the challenge is the way different paths of the setup code assume they are in charge (GUI vs. CLI vs. Flasher), which needs to be detangled. That's something that needs to be addressed if this code starts living in a different shared object (along with lots of places where commands call other commands as commands and pass string parameters as if they were entered with the CLI, but other commands separate that functionality properly).

I'll see what I can do about getting the comms code changes separated (though it may still be paired with flasher code changes because they really should be updated together).

[micolous]

Closing this out as I'm not working on this particular branch further, and it would require changing nearly every single source file in PM3.