Comms refactor (prerequisite of libproxmark work)

[micolous]

This is a refactor of exclusively the comms related code from #346, and a part of work for Issue #334:

• Move all globals into a single file (comms_globals.h)
• Fix an issue with proxmark3 built with Qt support quitting on startup
• flasher uses same code as proxmark3 client for communication with hardware
• Fixes a couple of other compilation issues on OSX

This appears to work with me on Linux and OSX with Qt and non-Qt targets.

[micolous]

This is currently blocking me from doing further work on libproxmark and porting Proxmark to Android. Aside from the merge conflicts, are there any outstanding issues with the style of this PR?

[pwpiwi]

Yes, it unnecessarily moves unrelated global variables to files where you never would expect them. Can't we have the comms refactoring only with this PR?

[micolous]

Part of the challenge of this porting work is that there are global variables in the first place. It makes it very difficult to isolate the client into a library at all, because I need to be able to entirely reconfigure the PM3 client's context without restarting the entire binary.

The point of moving them all into one file is that in the next stage, they'll be wrapped up into a pm3_connection or pm3_context struct. The PM3 protocol assumes that it can simply shuffle bits out of a shared buffer between it and the client, and many of the commands assume that they should pull data out of one of these buffers.

They need to be all declared exactly once, and right now they're declared multiple times depending on if you're using a GUI build of PM3 or not, and if you're using the GUI application or not. I have concerns that there are latent bugs in the existing implementation.

I'm going to skip the globals shuffling for now, and move that straight into passing a context struct around. However, it will make the context struct PR larger, and I suspect this debate will reopen.

[micolous]

Please take a look at dcd394e.

[pwpiwi]

On Ubuntu 14.04.5 LTS I get

comms.c: In function ‘uart_receiver’:
comms.c:187:4: warning: implicit declaration of function ‘msleep’ [-Wimplicit-function-declaration]
    msleep(10);
    ^

Should be fixed.

[pwpiwi]

If I am not mistaken, most of the "globals" in 'comms_globals.h` can be made static.

[micolous]

implicit msleep

Fixed

If I am not mistaken, most of the "globals" in 'comms_globals.h` can be made static.

They could (and I'm aware what I'm doing now is a hack), however this would mean that the code would need to be repeated across the different contexts that comms_globals.h is used. My intent is that they are declared exactly once, and that the migration to a pm3_context struct (next PR) will delete comms_globals.h.

[pwpiwi]

I am still not sure where this will end and I don't think that the discussion with the other maintainers really came to an end already.

Therefore please don't assume this to be an "intermediate" PR. I would like to accept it because the refactoring makes sense. But not with a separate comms_globals.h.

[micolous]

You can't have a static variable in C which is accessed in more than one file. I have written 24bca6a which removes comms_globals.h and still allows access to the globals that are used outside of the context of comms.c.

[pwpiwi]

Good! On which platforms did you test?

[micolous]

• Ubuntu 16.04 LTS (x86_64)
• macOS 10.12 (x86_64)

[pwpiwi]

Tested on Kali and Windows 10.
Compiling and linking showed no warnings and no errors.

Runs fine on Kali, on Windows however there is an issue. When running hw status several times, at some time the client starts to show error messages "Status command failed. USB Speed Test timed out" and then hangs (Ctl-C can still be used to stop the client).

~/github/proxmark3$ client/proxmark3.exe com6
Prox/RFID mark3 RFID instrument
bootrom: master/v3.0.1-22-g2a7861e-suspect 2017-06-27 19:35:06
os: fix_mfsim/v3.0.1-95-g8c248b5-dirty-suspect 2017-10-11 18:39:37
LF FPGA image built for 2s30vq100 on 2015/03/06 at 07:38:04
HF FPGA image built for 2s30vq100 on 2017/07/13 at 08:44:13

uC: AT91SAM7S256 Rev B
Embedded Processor: ARM7TDMI
Nonvolatile Program Memory Size: 256K bytes. Used: 197884 bytes (75%). Free: 64260 bytes (25%).
Second Nonvolatile Program Memory Size: None
Internal SRAM Size: 64K bytes
Architecture Identifier: AT91SAM7Sxx Series
Nonvolatile Program Memory Type: Embedded Flash Memory
proxmark3> hw stat
#db# Memory
#db#   BIGBUF_SIZE.............40000
#db#   Available memory........40000
#db# Tracing
#db#   tracing ................1
#db#   traceLen ...............0
#db# Fgpa
#db#   mode....................HF
#db# LF Sampling config:
#db#   [q] divisor:           95
#db#   [b] bps:               8
#db#   [d] decimation:        1
#db#   [a] averaging:         1
#db#   [t] trigger threshold: 0
#db# USB Speed:
#db#   Sending USB packets to client...
#db#   Time elapsed:      1500ms
#db#   Bytes transferred: 751616
#db#   USB Transfer Speed PM3 -> Client = 501077 Bytes/s
#db# Various
#db#   MF_DBGLEVEL......2
#db#   ToSendMax........201997388
#db#   ToSendBit........0
proxmark3> hw stat
#db# Memory
#db#   BIGBUF_SIZE.............40000
#db#   Available memory........40000
#db# Tracing
#db#   tracing ................1
#db#   traceLen ...............0
#db# Fgpa
#db#   mode....................HF
#db# LF Sampling config:
#db#   [q] divisor:           95
#db#   [b] bps:               8
#db#   [d] decimation:        1
#db#   [a] averaging:         1
#db#   [t] trigger threshold: 0
#db# USB Speed:
#db#   Sending USB packets to client...
#db#   Time elapsed:      1500ms
#db#   Bytes transferred: 754688
#db#   USB Transfer Speed PM3 -> Client = 503125 Bytes/s
#db# Various
#db#   MF_DBGLEVEL......2
#db#   ToSendMax........201997388
#db#   ToSendBit........0
proxmark3> hw stat
#db# Memory
#db#   BIGBUF_SIZE.............40000
#db#   Available memory........40000
#db# Tracing
#db#   tracing ................1
#db#   traceLen ...............0
#db# Fgpa
#db#   mode....................HF
#db# LF Sampling config:
#db#   [q] divisor:           95
#db#   [b] bps:               8
#db#   [d] decimation:        1
#db#   [a] averaging:         1
#db#   [t] trigger threshold: 0
#db# USB Speed:
#db#   Sending USB packets to client...
#db#   Time elapsed:      1500ms
#db#   Bytes transferred: 753152
#db#   USB Transfer Speed PM3 -> Client = 502101 Bytes/s
#db# Various
#db#   MF_DBGLEVEL......2
#db#   ToSendMax........201997388
#db#   ToSendBit........0
proxmark3> hw stat
#db# Memory
#db#   BIGBUF_SIZE.............40000
#db#   Available memory........40000
#db# Tracing
#db#   tracing ................1
#db#   traceLen ...............0
#db# Fgpa
#db#   mode....................HF
#db# LF Sampling config:
#db#   [q] divisor:           95
#db#   [b] bps:               8
#db#   [d] decimation:        1
#db#   [a] averaging:         1
#db#   [t] trigger threshold: 0
#db# USB Speed:
#db#   Sending USB packets to client...
#db#   Time elapsed:      1500ms
#db#   Bytes transferred: 753664
#db#   USB Transfer Speed PM3 -> Client = 502442 Bytes/s
#db# Various
#db#   MF_DBGLEVEL......2
#db#   ToSendMax........201997388
#db#   ToSendBit........0
proxmark3> hw stat
#db# Memory
#db#   BIGBUF_SIZE.............40000
#db#   Available memory........40000
#db# Tracing
#db#   tracing ................1
#db#   traceLen ...............0
#db# Fgpa
#db#   mode....................HF
#db# LF Sampling config:
#db#   [q] divisor:           95
#db#   [b] bps:               8
#db#   [d] decimation:        1
#db#   [a] averaging:         1
#db#   [t] trigger threshold: 0
#db# USB Speed:
#db#   Sending USB packets to client...
#db#   Time elapsed:      1500ms
#db#   Bytes transferred: 754688
#db#   USB Transfer Speed PM3 -> Client = 503125 Bytes/s
#db# Various
#db#   MF_DBGLEVEL......2
#db#   ToSendMax........201997388
#db#   ToSendBit........0
proxmark3> hw stat
#db# Memory
#db#   BIGBUF_SIZE.............40000
#db#   Available memory........40000
#db# Tracing
#db#   tracing ................1
#db#   traceLen ...............0
#db# Fgpa
#db#   mode....................HF
#db# LF Sampling config:
#db#   [q] divisor:           95
#db#   [b] bps:               8
#db#   [d] decimation:        1
#db#   [a] averaging:         1
#db#   [t] trigger threshold: 0
#db# USB Speed:
#db#   Sending USB packets to client...
#db#   Time elapsed:      1500ms
#db#   Bytes transferred: 752128
#db#   USB Transfer Speed PM3 -> Client = 501418 Bytes/s
#db# Various
#db#   MF_DBGLEVEL......2
#db#   ToSendMax........201997388
#db#   ToSendBit........0
proxmark3> hw stat
#db# Memory
#db#   BIGBUF_SIZE.............40000
#db#   Available memory........40000
#db# Tracing
#db#   tracing ................1
#db#   traceLen ...............0
#db# Fgpa
#db#   mode....................HF
#db# LF Sampling config:
#db#   [q] divisor:           95
#db#   [b] bps:               8
#db#   [d] decimation:        1
#db#   [a] averaging:         1
#db#   [t] trigger threshold: 0
#db# USB Speed:
#db#   Sending USB packets to client...
#db#   Time elapsed:      1500ms
#db#   Bytes transferred: 753664
#db#   USB Transfer Speed PM3 -> Client = 502442 Bytes/s
#db# Various
#db#   MF_DBGLEVEL......2
#db#   ToSendMax........201997388
#db#   ToSendBit........0
proxmark3> hw stat
#db# Memory
#db#   BIGBUF_SIZE.............40000
#db#   Available memory........40000
#db# Tracing
#db#   tracing ................1
#db#   traceLen ...............0
#db# Fgpa
#db#   mode....................HF
#db# LF Sampling config:
#db#   [q] divisor:           95
#db#   [b] bps:               8
#db#   [d] decimation:        1
#db#   [a] averaging:         1
#db#   [t] trigger threshold: 0
#db# USB Speed:
#db#   Sending USB packets to client...
Status command failed. USB Speed Test timed out
proxmark3> hw stat
Status command failed. USB Speed Test timed out
proxmark3> hw stat

[iceman1001]

My humble tests of parts of this changes has left me with a somewhat dying client after a while.
I have a hard time decide If this depends on this code or my other changes

[micolous]

@pwpiwi Windows however there is an issue. When running hw status several times, at some time the client starts to show error messages "Status command failed. USB Speed Test timed out" and then hangs (Ctl-C can still be used to stop the client).

I tried to repro this on Linux, and couldn't. I note you're on a slightly different firmware, and this probably shouldn't have any impact, but I would feel more assured if you were on exactly the same firmware.

I tried to run with gator91600's binaries (without my modifications, specifically this build), in a virtual machine running Windows 7.

Using USB based sharing to the guest in VirtualBox, I was unable to even get the client to even connect to my PM3. The client just reports "invalid serial port" and then Windows reports This device cannot start. (Code 10) a few seconds after the device is connected.

I also tried sharing to the Windows guest as a serial port (rather than USB device), again running gator91600's binaries, and saw the "USB Speed Test timed out" message you did when running hw stat, pretty much immediately. However, hw tune returned sensible results, even though the USB tests timed out, and the device continued to work.

C:\PM3_WIN3\win32>proxmark3.exe com1
Prox/RFID mark3 RFID instrument
bootrom: master/v2.2 2015-07-31 11:28:11
os: comms-refactor/v2.2.0-598-g24bca6a2-suspect 2017-10-10 10:21:43
LF FPGA image built for 2s30vq100 on 2015/03/06 at 07:38:04
HF FPGA image built for 2s30vq100 on 2017/07/13 at 08:44:13

uC: AT91SAM7S256 Rev B
Embedded Processor: ARM7TDMI
Nonvolatile Program Memory Size: 256K bytes. Used: 198877 bytes (76%). Free: 63267 bytes (24%).
Second Nonvolatile Program Memory Size: None
Internal SRAM Size: 64K bytes
Architecture Identifier: AT91SAM7Sxx Series
Nonvolatile Program Memory Type: Embedded Flash Memory
proxmark3> hw stat
#db# Memory
#db#   BIGBUF_SIZE.............40000
#db#   Available memory........40000
#db# Tracing
#db#   tracing ................1
#db#   traceLen ...............0
#db# Fgpa
#db#   mode....................HF
Status command failed. USB Speed Test timed out
proxmark3> hw stat
#db# Memory
#db#   BIGBUF_SIZE.............40000
#db#   Available memory........40000
#db# Tracing
#db#   tracing ................1
#db#   traceLen ...............0
#db# Fgpa
Status command failed. USB Speed Test timed out
proxmark3> hw tune

Measuring antenna characteristics, please wait.........
# LF antenna: 26.26 V @   125.00 kHz
# LF antenna: 44.14 V @   134.00 kHz
# LF optimal: 44.14 V @   129.03 kHz
# HF antenna:  0.92 V @    13.56 MHz
# Your HF antenna is unusable.
Displaying LF tuning graph. Divisor 89 is 134khz, 95 is 125khz.

I suspect there may be something else at play which causes this issue, but I'm also not able to reproduce this command actually working properly on a mainline client on Windows.

If you can be very certain that you're not able to reproduce the same symptoms on Windows without my patches, I will have a go at getting a real Windows environment running and some cross-compiler for it. But that is a huge time investment.

This should also be documented in a testing procedure somewhere, because this is not a command I would consider running -- I was doing regular card operations which are significantly less stressful.

@iceman1001 My humble tests of parts of this changes has left me with a somewhat dying client after a while.
I have a hard time decide If this depends on this code or my other changes

What platform is this, and can you please run with a clean build, using firmware that matches my version? I don't have enough information to troubleshoot this, and I'm aware your branch has a different protocol to mainline.

[micolous]

I've refactored to take into account recent changes in #414 (and squashed commits), but am still blocked needing more info about the other Windows issues (because I don't think they're related to my changes).

[pwpiwi]

I tried to repro this on Linux, and couldn't.

Issue is on Windows only.

I suspect there may be something else at play which causes this issue, but I'm also not able to reproduce this command actually working properly on a mainline client on Windows.

The error message is quite common, because the speed test runs for 1500ms which is exactly the timeout for the client waiting for a response. However usually the results are displayed nevertheless and I found no issue with this command without your commit.

To be sure, I will provide a patch for the speed test timeout first and then test again.

[pwpiwi]

I have raised PR #434 which should fix the timeout with hw status in Windows. Please fix your merge conflicts and I will test your PR again.

[micolous]

Merge conflicts from #417 resolved, and I cherry-picked your patch from #434, as the changes didn't apply cleanly. Seems to be happy on Linux and OSX.

[micolous]

Merge conflicts resolved again because #434 was merged.

[cjbrigato]

Any insight performance-wise from this commit ?

[pwpiwi]

Observed the same as you did: flashing is much slower on Kali. Output of hw status varies.

[Fl0-0]

I have the issue #449