First try at disconnect/reconnect detection #516

uzlonewolf · 2017-12-11T10:06:03Z

No idea if it's going to work on WIN32... If ReadFile() returns True on a no-data timeout then it'll work, otherwise we'll need to call GetLastError() and check what error was returned. Can someone running WIN32 try and let me know?

… work on WIN32.

uzlonewolf · 2017-12-11T10:09:21Z

Oh yeah, the changes to cmdhw.? are because I want to print the HW info on reconnect but cannot call CmdVersion() directly due to being in the wrong thread.

pwpiwi · 2018-02-05T07:02:47Z

client/proxmark3.c

+			offline = 0;
+			//CmdVersionW(NULL, true);
+			clearCommandBuffer();
+			uart_send(sp, (byte_t*) &version_cmd, sizeof(UsbCommand)); // request it from the HW


This would destroy all data in BigBuf[] when reconnecting. I think the major purpose of the possibility to reconnect is to support offline functionality like sniffing some data?

As it is now, unplugging causes the CPU to shoot to 100% used and the only way to continue what you're doing is to exit and restart the client. When I'm on my laptop I sometimes need to move USB ports or occasionally it accidentally comes unplugged, and needing to restart is annoying.

At any rate I've seen some odd segfaults in readline with this, so I was thinking about ripping out the "re-get version info" stuff anyway.

Also, as far as I can tell clearing the command buf doesn't touch BigBuf[]. Not sure if the act of disconnecting/reconnecting the USB cable will do that, though without a battery whatever's in the hardware will be gone.

Indeed, clearCommandBuffer() doesn't touch BigBuf[] but querying for the version info does. This was the main reason to cache the version info on client side. I see two possible solutions:

cache the version info on device side instead of client side

don't query the version info on reconnect

The latter is obviously easier to implement.

Newest version still has clearCommandBuffer() but instead of querying the version on re-connect it simply clears CmdVersion()'s cache.

iceman1001 · 2018-02-17T16:29:53Z

@uzlonewolf Would you mind addressing @pwpiwi 's review?
I would really like to have this functionality in the upcoming release.

…'t overwrite BigBuf

uzlonewolf · 2018-02-17T19:20:08Z

Version retrieval on re-connect removed. Newest version still has clearCommandBuffer() but instead of querying the version on re-connect it simply clears CmdVersion()'s cache so the next "hw ver" will fetch it from hardware.

pwpiwi · 2018-02-17T19:51:36Z

The issue is not really fixed. The next "hw ver" will then destroy BigBuf. I suggest that you simply do nothing with the version info on reconnect. Someone may implement the version cache on device side later...

uzlonewolf · 2018-02-17T20:52:06Z

My issue is if someone connects a 2nd PM3, or unplugs a single PM3 and updates the firmware on a different computer, the info returned by "hw ver" will be wrong. I would prefer to just add a note to the "hw ver" help text noting that it will overwrite BigBuf if it's run after a disconnect/reconnect. If you insist on not clearing the cache then I would like to add a optional flag to "hw ver" to tell it to ignore the cache, say "hw ver [nocache]"

… ver" to force cache clearing

uzlonewolf · 2018-02-18T00:12:17Z

Got basically done adding version caching to armsrc, but then realized there was no malloc() :-/ Not sure what the best way to finish this is, I can statically allocate 80 bytes (40 to each, LF+HF) but that seems wasteful with the limited SRAM when the actual version string is going to be closer to 56 bytes (28+28). As such the above push has no armsrc changes.

iceman1001 · 2018-02-18T03:23:32Z

The pm3 client is built around the concept of working against one proxmark device at a time.
When ever I use several pm3, I use several terminal windows with clients and reconnect against their tty.

As I see it, the reconnect functionality is to enable downloading of the collected data if device has been running on battery, the device version data is of no interest for that.

So, simplest function for that is to ignore version data when reconnect. I suggest you go with@pwpiwi recommendation.

iceman1001 · 2018-02-18T03:25:04Z

In iceman fork, I use "s - silence" or you could even use " v - verbose" , to specifiy that you want to print version data available (cached) but when reconnect then just don't print it...

uzlonewolf · 2018-02-18T04:26:40Z

Yes, that last push, 6e0b2d0, gets rid of all the version printing/cache clearing on reconnect and instead adds an optional argument to the "hw ver" command to tell it you want un-cached data from the hardware. Call "hw ver" and it acts just like it's always done. Call "hw ver nocache" and it invalidates the cache and queries the hardware.

micolous · 2018-02-18T09:50:20Z

client/proxmark3.c

@@ -70,16 +71,52 @@ byte_t* prx = rx;
 static void *uart_receiver(void *targ) {


Hey, can you please hold up on this until #463 is in? Aside from not wanting to have to go through a merge conflict process again, I address some of the reconnection issues already in that for flasher.

micolous · 2018-02-18T09:51:04Z

client/proxmark3.c

-			prx += rxlen;
-			if (prx-rx < sizeof(UsbCommand)) {
+		if( need_reconnect ) {
+			sp = uart_open(sp_name);


This will break on Android. Is there a better way to pass this from within the main() function instead?

See https://github.com/AndProx/AndProx/blob/b662c653d7ba33b570d71e34c949e3a8e7f17d8e/natives/src/main/cpp/natives.c#L82

Currently, this assumes that the "serial port identifier will always be a string". This isn't true if uart_open is replaced.

For each of uart_win32.c and uart_posix.c, add a char* referring to the serial port path in their platform-specific serial_port structs.

In uart.h, add a method bool uart_reconnect(serial_port* sp).

uart_reconnect can then be implemented in platform-specific ways, that takes in the serial port identifier stashed away earlier, and will setup the existing serial_port struct to work again. This returns true on success, or false on error.

Whenever the application determines the need to reconnect, it calls uart_reconnect, which can operate in-place. uart_receiver can have some retry logic.

I have no idea what part of the above code will fail on Android, that link doesn't explain it. The original PM3 code takes main()'s argv[1] and feeds it to uart_open(). sp_name is just a copy of argv[1]. In what cases will argv[1] not be a char* ? If main() is changed to not feed argv[1] into uart_open then obviously the other instances of uart_open() will need to be changed too. Surely replacing "char*" with whatever becomes appropriate at that time isn't that difficult...

The entire point of this patch is to simply detect a disconnection and attempt to reopen the same port. It is not to rewrite the entire uart system.

All of it will fail on Android, because the "serial port" is a reference to a bunch of JNI object IDs, which allow it to pass uart_send and uart_receive to Java method calls.

This is implemented in these files:

https://github.com/AndProx/AndProx/blob/ae58e617cfd033f2d3ac94046a89291c790002a2/natives/src/main/cpp/natives.c#L122-L214

https://github.com/AndProx/AndProx/blob/master/natives/src/main/cpp/uart_android.c

https://github.com/AndProx/AndProx/blob/master/natives/src/main/java/au/id/micolous/andprox/natives/NativeSerialWrapper.java

PM3 doesn't get called by main either, so there is no argv. The link I sent is one of the bootstrap functions which sets up PM3 similar to how main would.

This is done because there is no /dev/ttyACM0 except on rooted devices where one has installed the cdc_acm kernel module. Everything is passed around the Android USB Host API. Requiring root is a major blocker to use on the platform.

Having an API call for "reconnect" means that it doesn't matter how uart is implemented, whether it's a string pointing to a device name, it's wrapped up in some other platform specific code, or it's part of some testing harness.

At least on Android I'd bring "reconnect" up into Java space, hit the USB stack on the head a few times, and then pass it back.

As for precedent: @iceman1001 objected when I proposed removing uart_{get,set}_speed even though that's not actually used in mainline PM3, but was in his branch.

If that went in as-is, even when it was "internal", I would no longer be able to track upstream PM3 for Android until it was replaced with the API extensions I proposed.

micolous · 2018-02-18T09:54:10Z

uart/uart_posix.c

    // Read time-out
    if (res == 0) {
      if (*pszRxLen == 0) {
        // Error, we received no data
-        return false;
+        return true;


Why does this path also return true? Don't you want to know when there is an error?

Timing out without receiving any data may not be an error if the hardware just doesn't have any data ready for us at the moment.

uart_posix.c does some additional and unnecessary re-packaging which is already handled in proxmark3.c uart_receiver():

if (prx-rx < sizeof(UsbCommand)) { continue; }

uart_win32.c doesn't do that. Why not aligning/simplyfying the Unix version and make it behave like the Windows version?

Ah true. I also double checked ReadFile (the win32api function that this is equivalent to), that doesn't return false on timeout.

So that entire if statement can go.

The Win and *nix APIs are totally different. It's been a while since I looked at uart_posix.c and I really didn't spend a huge amount of time in there, but from what I recall the various return's are to break out of a loop or to handle the various conditions the select() can result in (read error vs "no data available" which isn't an error). If it's rewritten to be like uart_win32.c then it will block forever (or at least until the PM3 is unplugged) and not allow the comms thread to exit on client shutdown due to the "do { ... } while(byteCount);" . The only clean-up I see is the "if(..) return true; else return true;" As per my first comment I have absolutely no clue if the WIN32 routine returns an error on no data or not, or an error on a read error or not. No windoze boxen in this building...

FYI, if you want to test stuff on Windows, http://modern.ie/ has time-limited trial Windows virtual machines. They're ostensibly for testing websites in Internet Explorer and Edge, but you can install any software you like on them.

That should be sufficient for getting ProxSpace (the Windows build environment) going.

pwpiwi · 2018-08-14T08:18:23Z

Code has settled now. @micolous changes are included and hw ver no longer caches on client side. Would you rebase your change?

uzlonewolf · 2018-08-17T22:01:09Z

I can, but I'm out of town at the moment and it may be a while before I can get to it. I do have my PM3 with me so we'll see.

First try at disconnect/reconnect detection. No idea if it's going to…

96aab06

… work on WIN32.

uzlonewolf mentioned this pull request Dec 11, 2017

Detect when reader is unplugged rather than busy loop 100% CPU #509

Open

Fixed function name

2a3f94d

iceman1001 mentioned this pull request Jan 6, 2018

Next PM3 offical release. v.3.2.0 #501

Closed

pwpiwi reviewed Feb 5, 2018

View reviewed changes

Removed the retrieving of version information on re-connect so we don…

1ab4fc9

…'t overwrite BigBuf

Removed automatic version cache clearing, added [nocache] flag to "hw…

6e0b2d0

… ver" to force cache clearing

micolous suggested changes Feb 18, 2018

View reviewed changes

uzlonewolf mentioned this pull request Dec 29, 2019

speedup 'hf mf chk' #901

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First try at disconnect/reconnect detection #516

First try at disconnect/reconnect detection #516

uzlonewolf commented Dec 11, 2017

uzlonewolf commented Dec 11, 2017

pwpiwi Feb 5, 2018

uzlonewolf Feb 6, 2018

uzlonewolf Feb 6, 2018

pwpiwi Feb 8, 2018

uzlonewolf Feb 17, 2018

iceman1001 commented Feb 17, 2018

uzlonewolf commented Feb 17, 2018 •

edited

Loading

pwpiwi commented Feb 17, 2018

uzlonewolf commented Feb 17, 2018

uzlonewolf commented Feb 18, 2018 •

edited

Loading

iceman1001 commented Feb 18, 2018

iceman1001 commented Feb 18, 2018

uzlonewolf commented Feb 18, 2018

micolous Feb 18, 2018 •

edited

Loading

micolous Feb 18, 2018 •

edited

Loading

micolous Feb 18, 2018

uzlonewolf Feb 18, 2018

micolous Feb 19, 2018

micolous Feb 18, 2018

uzlonewolf Feb 18, 2018

pwpiwi Feb 18, 2018

micolous Feb 18, 2018

uzlonewolf Feb 18, 2018

micolous Feb 19, 2018 •

edited

Loading

pwpiwi commented Aug 14, 2018

uzlonewolf commented Aug 17, 2018

		@@ -70,16 +71,52 @@ byte_t* prx = rx;
		static void uart_receiver(void targ) {

First try at disconnect/reconnect detection #516

Are you sure you want to change the base?

First try at disconnect/reconnect detection #516

Conversation

uzlonewolf commented Dec 11, 2017

uzlonewolf commented Dec 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iceman1001 commented Feb 17, 2018

uzlonewolf commented Feb 17, 2018 • edited Loading

pwpiwi commented Feb 17, 2018

uzlonewolf commented Feb 17, 2018

uzlonewolf commented Feb 18, 2018 • edited Loading

iceman1001 commented Feb 18, 2018

iceman1001 commented Feb 18, 2018

uzlonewolf commented Feb 18, 2018

micolous Feb 18, 2018 • edited Loading

Choose a reason for hiding this comment

micolous Feb 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micolous Feb 19, 2018 • edited Loading

Choose a reason for hiding this comment

pwpiwi commented Aug 14, 2018

uzlonewolf commented Aug 17, 2018

uzlonewolf commented Feb 17, 2018 •

edited

Loading

uzlonewolf commented Feb 18, 2018 •

edited

Loading

micolous Feb 18, 2018 •

edited

Loading

micolous Feb 18, 2018 •

edited

Loading

micolous Feb 19, 2018 •

edited

Loading