Refactoring uart interface

[micolous]

This is part of a series of improvements planned for Issue #334.

• Removed the AES implementation from armsrc, as it is not used. The version in client remains.
• Moved legic_prng.* from common to armsrc, as it is never used by the client.
• Significantly refactored the UART related code. I've removed unused functionality (which wasn't even fully implemented in the win32 version), split those files up, changed the implementation slightly (to make it nicer to work with).
• Fixed a buffer overflow in the posix implementation of uart_receive. It would ignore the pbtRx parameter and just write as much as it could.
• Downsized a buffer in the proxmark client, saving 16MiB of RAM at runtime.

While this is a pretty large patch, most of it is moving around or deleting existing code.

[iceman1001]

The AES was there because of the long split between what shall be on device and on client? it came down to purpose, for standalone desfire support it is needed. but the standalone mode hasn't gotten some real love for awhile now
Same with legic stuff, shall it be on client or not. There was noone adding things on the client for legic..
I'm happy the usb-uart got some love. Much needed! I guess you need the division between posix-win for your libs idea

[iceman1001]

But I miss the uart-setspeed, since I use it in my fork..

[marshmellow42]

while i don't love losing flexibility for users to implement standalone modes and do as they wish, i don't think AES will contribute to that mode greatly at this time. the code is in the history so it can always be recalled.

legic stuff. fine it just moved it could be moved back if someone needed it.

the uart stuff appears minor, but should be verified with testing on all OS.
i know there are many schools of thought on "dead" functions. personally i'm of the opinion if it is for functionality not currently used but "May be useful in the future" then it should be left. how to define what may be useful in the future may be difficult.
but the setspeed and getspeed is one of those in my book that may be useful (could be commented out though)

but i know many will disagree with me on leaving "dead" code.

[iceman1001]

Correct me if I'm wrong but don't compilers remove "unused" functions nowdays?
If so, then the removal becomes just cosmetic.. well, for changes, you need to remember to change in the same function for multiple OS implementation files.

I mentioned in another thread that maybe all @micolous ideas of refactoring should go into a new branch, instead of Master branch. If the branch "libpm3" becomes popular we can have a discussion about replacing pm3 master with it.

[marshmellow42]

i agree that at some point @micolous ideas should be split to a new branch.
i don't however see any reason these particular changes need to go into a new branch and not the master. as there are potentially some good fixes here.

[iceman1001]

besides removing unused code... and removing of uart-setspeed.
well, I suggest closing this PR and @micolous make a new one with uart-setspeed left.. or update this pr?!?..
then it would be ok to merge.
I'm guessing with all these new changes I'll never manage to merge my fork haha.

[iceman1001]

or why not a libpm3 repo?

[marshmellow42]

never say never! ;) there are many things in your fork that will be useful to have in the master.

and i think we will have to have more discussion around how @micolous sees his code split. adding a branch could make merging it in the future more complicated if there are lots of other changes at the same time... (but it may be safer for the master branch)

making a new repo would make merging the changes even more difficult.

and unless the plan is to ultimately merge it i think it kind of breaks his hope that ongoing improvements get implemented in a way that works with his new form...

[iceman1001]

I see no reason for a libpm3 should be merged anyway? It would live its own life.
Good things would come into pm3 master and lib-related items would stay in libpm3.

As for me, I would like to see the PM3 Master to be stable. This lib-idea is a development track, which would fit better in its own repo. Basically what I'm saying Im lazy, I don't want to spend time fixing bugs that will come from these changes since this libpm3 are of no use for me. Speaking personally.

[marshmellow42]

let's continue on #334 :)

[marshmellow42]

@micolous can you add back in the uart_set_speed and uart_get_speed for this pull request?
i will continue testing it after that.

Thanks!

[micolous]

I'll add it back in, but I will note that the USB interface exposed by common/usb_cdc.c is virtualised, so setting baud rates shouldn't really have any effect. It may have impact on the OS level, though.

[micolous]

OK, this is now ready for review.

Ooh, need to revert some more stuff

[micolous]

Per discussion in #334, I have reverted the deletion of AES related code. This PR now only contains code for the uart refactor/cleanup.

[iceman1001]

I second the merge of this. Great work giving some love to this part of the code!

[marshmellow42]

It looks good but I have not had time to flash it and test any commands yet. Has anyone else?

[pwpiwi]

Same with me. Code rewiewed and should be OK. I may find some time tonight to compile and test.

[iceman1001]

@micolous
since we are at it, have a look a this one from icemanfork. With this one I got the comspeed up on my virtual windows machines. Might as well add it now.

iceman1001@5ed5e41

[pwpiwi]

I am getting warnings on Kali Linux:

flasher.c: In function ‘CloseProxmark’:
flasher.c:68:3: warning: implicit declaration of function ‘unlink’ [-Wimplicit-function-declaration]
   unlink(serial_port_name);
   ^~~~~~
flasher.c: In function ‘main’:
flasher.c:131:5: warning: implicit declaration of function ‘sleep’ [-Wimplicit-function-declaration]
     sleep(1);
     ^~~~~

This should be fixed.

[marshmellow42]

Hmm. I don't remember seeing those warnings before but I also don't see where his changes caused them...

[pwpiwi]

We had #include <unistd.h> in uart.h. Need to add it to flasher.c now.

[marshmellow42]

Yes, now I see it. Sorry.

[marshmellow42]

ok so add at line 22 (flasher.c):

#else 
# include <unistd.h>

and change sleep(1); to msleep(1000);
look ok?

[marshmellow42]

once i make those changes windows and ubuntu appears to work fine. (ran a few simple lf search && hf search tests)

[marshmellow42]

i hope no one is offended i took the initiative and fixed the linux warnings.
i believe this should be ready to merge.

i tested and flashed on mingw and ubuntu 14.04 (but did not test on osx)

[cjbrigato]

about the "what goes were" it is quite simple.

• device is a very small arm which is basically a handy frontend to the fpga. With this propriety and aim, the arm has to never use "RTOS" in order to be anyhow "really" RT, and it has to never or exceptionnaly have unkown delays of operations. Since transactions boundaries are know in every way, and no Threading nor any other form of multiplexing/Scheduling task is here, it is made for and a goal for it to produce near the very same result in any same context.
• Anything requiring compute is to go to client. Any "clientable" thing that is far easier to make device-able and which is reducing either communication or code execution is to be implemented on device or proposed as tradeof. Emulator memory is such an exemple.
  The case of AES on device is non-event here. It's there, it takes nothing to less to other, it's ready. It uses no ressource to be there. And since the choice has been made to encapsulate the memory management in a Fixed and warranted "BigBuf", there is again nothing to gain in this matter.

Would benefit from another ticket

Again about AES haven't seen the code but there is anoter thing to consider : ARM7TDMI's brother AT91SAM7S-EK revealed an hidden crypto hardware engine in fine possible to activate (just a driver to make and some translation between adresses of real model doc, if anything). So the "Crypto Engine No" is a lie on hardware specs. This is old news for quite old MCU so can't say if it's really implementable as easy as it seems.