Proxmark3 community

iZsh

Contributor

Registered: 2010-01-02

Posts: 95

Questions about code cleaning

Hello,

I've been doing some code cleaning/restructuring in my sandbox on the client side recently, and I have a couple of questions:

- What's the difference between hisamples and hisampless?
And please don't tell me the latter is signed
I'm asking about real differences, looking at the code, not the help message.
As far as I can see from the code, apart from an optional parameter (which is actually optional, not mandatory, if you look at the code, unlike what the help is saying), the code seems to be doing the exact same thing. I'm about to trash hisamples in favor of hisampless (with renaming of course) since it looks like it's backward-compatible, but I want to be sure I'm not missing something here.

- losamples seems to be doing almost the same thing except a -128 correction on the fly. I could dig deeper in the code, but if someone could quickly clarify why that would save me some times

- higet calls CmdHi14read_sim (not confusing at all...) which uses a CMD_ACQUIRE_RAW_ADC_SAMPLES_ISO_14443_SIM command, which now seems to be a dead opcode. I'm about to trash this command as well, but I want to be sure I'm not losing it either...

- hisamplest calls CmdHi14readt (still not confusing at all), which first uses a CMD_ACQUIRE_RAW_ADC_SAMPLES_ISO_14443 command and then calls CmdHisamplest. This subsequent function executes right away, like hisamples/hisampless, a CMD_DOWNLOAD_RAW_ADC_SAMPLES_125K command.
So it seems hisamplest is like calling hi14read first, download the samples (using the usual CMD_DOWNLOAD_RAW_ADC_SAMPLES_125K command) and then it does some magic with the data. There is a switch/case but it seems to be hardcoded to the first case anyway...
Can someone clarify what this code is doing and what was the intent (which could be different)?

BTW, if you guys could refrain from committing in the client side for a few more days (until I'm done with it), that would prevent from merge nightmares ^^

Anyway, any insights would be appreciated

Best Regards

d18c7db

Contributor

Registered: 2008-08-19

Posts: 292

Re: Questions about code cleaning

Yeah the current code for hisamples and hisampless is functionally the same and they could be merged into one. Same with losamples. I think they should all be merged into one and add maybe a separate generic buffer function for adding a constant to the buffer maybe. Haven't got the time to look at the hi* commands now but you seem to be on the right track.

d18c7db

Contributor

Registered: 2008-08-19

Posts: 292

Re: Questions about code cleaning

Continuing on, losamples turns the unsigned values in the buffer (range 0..255) into signed values (range -128..127) hence the -128 correction you mention. I believe we're better off with a single "transfer buffer" command then a number of buffer processing commands.

higet - can be trashed, there's no code on the PM3 handling it.

hisamplest - not sure what the intent was there. No one has worked on this since it was first checked into the SVN.

iZsh

Contributor

Registered: 2010-01-02

Posts: 95

Re: Questions about code cleaning

I trashed hisamplest. We can get it back from svn anyway if we ever need it.
I didn't factorize the lf and hf sample commands yet. That will be stage 2

I just committed my changes, which took me a few (full) day to complete. I probably broke some stuff, since I haven't done much testing apart from compiling. So expect some turbulence while I/we iterate on the cleaning/restructuring. Hopefully I didn't break too many things though and you guys will like these changes.

I wish people won't be too lazy in the future and will really create new files/subcommands whenever they add support for a new RFID type instead of throwing everything in the lf/hf command tables... Otherwise all the efforts to clean the code would be wasted and I'd be very sad.

Here is my changelog (that I posted in svn anyway). Feel free to comment.

Client cleanup and restructuring. Stage 1...
Next Step is refactoring some of the giant functions which are
just copy/paste of some other ones with just a few line changes,
removing unnecessary 'goto' etc.

The MS Windows version is broken with this commit but will be fixed
soon. Everything can't be done all at once

The commands are now hierarchical, for example:
"hf 14a read" vs. "hf 14b read".
You can also request help:
"hf help", "data help", "hf 15 help" etc.

Indents are now space-based, not tab-based anymore. Hopefully
no one will be trolling about it, considering the suicide-prone work
being done here

client/cmdhw.c, client/proxusb.c, client/cmdhw.h, client/proxusb.h,
client/cmdmain.c, client/cmdlfhid.c, client/cmdmain.h, client/cmdlfhid.h,
client/data.c, client/data.h, client/cmdhf.c, client/cmdlf.c,
client/cmdhf.h, client/cmdhf15.c, client/cmdhf14b.c, client/cmdlf.h,
client/cmdhf15.h, client/cmdhf14b.h, client/cmddata.c, client/cmddata.h,
client/ui.c, client/cmdparser.c, client/cmdlfti.c, client/ui.h,
client/cmdlfem4x.c, client/cmdparser.h, client/cmdlfti.h, client/cmdlfem4x.h,
client/graph.c, client/graph.h, client/cmdhf14a.c, client/cmdhf14a.h,
client/cmdhflegic.c, client/cmdhflegic.c: New files.

client/cli.c, client/flasher.c, client/snooper.c, client/proxmark3.c,
client/proxmark3.h, client/Makefile: Update accordingly.

client/flash.h, client/flash.c, client/proxgui.cpp: Cosmetic changes.

client/translate.h, client/command.c, client/gui.c,
client/usb.c, client/prox.h: Remove.

include/usb_cmd.h (CMD_ACQUIRE_RAW_ADC_SAMPLES_ISO_14443_SIM): Remove dead cmd.

common/crc16.h: New file.
common/crc16.c: Modify accordingly.
common/iso14443crc.h: New file.
common/iso14443_crc.c: Rename to
common/iso14443crc.c: and modify accordingly.

armsrc/lfops.c, armsrc/iso14443.c,
armsrc/iso14443a.c: include .h files from
the common directory instead of including the c files.

common/Makefile.common, armsrc/Makefile: Modify accordingly.

henryk

Contributor

Registered: 2009-07-27

Posts: 99

Re: Questions about code cleaning

Thanks iZsh for your great work so far!

One inquiry: Would anybody mind changing the -std=c99 in client/Makefile to -std=gnu99, thereby enabling GNU extensions?

This is necessary for compiling against libusb-0.1.* since its usb.h makes use of u_int*_t and PATH_MAX, both of which are not defined unless the GNU extensions are enabled.

iZsh

Contributor

Registered: 2010-01-02

Posts: 95

Re: Questions about code cleaning

Not at all, you even solved the mystery: http://code.google.com/p/proxmark3/source/detail?r=318

I added -std=c99 initially just because I couldn't stand stuff like
int i;
for (i ...

instead of for
(int i =..

I just committed the changes

d18c7db

Contributor

Registered: 2008-08-19

Posts: 292

Re: Questions about code cleaning

iZsh, just wondering, are you still working on getting the windows client compiling again  ?

iZsh

Contributor

Registered: 2010-01-02

Posts: 95

Re: Questions about code cleaning

kind of, I just got busy with other stuff. I'll try to fix it this weekend

d18c7db

Contributor

Registered: 2008-08-19

Posts: 292

Re: Questions about code cleaning

I've commited some changes that get the windows client to compile again, someone needs to test that the *nix side is still OK.

Last edited by d18c7db (2010-02-18 10:11:06)

adam@algroup.co.uk

Contributor

From: UK

Registered: 2009-05-01

Posts: 203

Website

Re: Questions about code cleaning

Nope:

cc -std=gnu99 -I. -I../include -I../common -I/opt/local/include -Wall -Wno-unused-function  -g3 -DHAVE_GUI   -c -o proxmark3.o proxmark3.c
In file included from proxusb.h:13,
                 from proxmark3.c:7:
../include/usb.h:69:23: error: pshpack1.h: No such file or directory
../include/usb.h:290:21: error: poppack.h: No such file or directory
make[1]: *** [proxmark3.o] Error 1

atrox

Contributor

Registered: 2010-01-08

Posts: 35

Re: Questions about code cleaning

My patch makes it compile again under linux, but there still seem to be some usb-problems

--- proxmark3-read-only/client/flash.c  2010-02-19 15:06:21.000000000 +0100
+++ proxmark3-read-only.atrox/client/flash.c  2010-02-19 15:19:11.000000000 +0100
@@ -5,6 +5,7 @@
 BOOL UsbConnect(void);
 #else
 #include <proxusb.h>
+#include <stdlib.h>
 #endif
 
 #include <usb_cmd.h>
@@ -18,7 +19,11 @@
 
 static uint32_t ExpectedAddr;
 static uint8_t QueuedToSend[256];
+#ifdef WIN32
 static BOOL AllWritten;
+#else
+static bool AllWritten;
+#endif
 #define PHYSICAL_FLASH_START 0x100000
 #define PHYSICAL_FLASH_END   0x200000
 
diff -u -x .elf -x .d -x '*.o' -x .svn -r proxmark3-read-only/client/Makefile proxmark3-read-only.atrox/client/Makefile
--- proxmark3-read-only/client/Makefile  2010-02-19 15:06:21.000000000 +0100
+++ proxmark3-read-only.atrox/client/Makefile  2010-02-19 14:59:54.000000000 +0100
@@ -12,8 +12,8 @@
 QTLDLIBS = $(shell pkg-config --libs QtCore QtGui 2>/dev/null)
 
 CMDSRCS = \
-      $(VPATH)\crc16.c \
-      $(VPATH)\iso14443crc.c \
+      $(VPATH)/crc16.c \
+      $(VPATH)/iso14443crc.c \
       data.c \
       graph.c \
       ui.c \
@@ -59,6 +59,12 @@
 CLEAN = prox.exe
 endif
 
+

 all: $(BINS)
 
 all-static: LDLIBS:=-static $(LDLIBS)
diff -u -x .elf -x .d -x '*.o' -x .svn -r proxmark3-read-only/include/usb.h proxmark3-read-only.atrox/include/usb.h
--- proxmark3-read-only/include/usb.h  2010-02-19 15:06:17.000000000 +0100
+++ proxmark3-read-only.atrox/include/usb.h  2010-02-19 14:46:37.000000000 +0100
@@ -65,9 +65,10 @@
 #define USB_DT_HUB_NONVAR_SIZE    7
 
 
+#ifdef WIN32
 /* ensure byte-packed structures */
 #include <pshpack1.h> 
-
+#endif
 
 /* All standard descriptors have these 2 fields in common */
 struct usb_descriptor_header {
@@ -287,8 +288,9 @@
 extern struct usb_bus *usb_busses;
 
 
+#ifdef WIN32
 #include <poppack.h>
-
+#endif
 
 #ifdef __cplusplus
 extern "C" {

Last edited by atrox (2010-02-19 15:33:30)

iZsh

Contributor

Registered: 2010-01-02

Posts: 95

Re: Questions about code cleaning

Does it really need to be a bool instead of int? Let's try to keep the #ifdef to a really minimum amount.

atrox

Contributor

Registered: 2010-01-08

Posts: 35

Re: Questions about code cleaning

since i had no time understanding all the changes that have been made, i simply put an #ifdef around the incompatible parts.  Besides "AllWritten" looks fishy to me, it is only accessed writing, but nowhere read.

still, usb support is broken. it does not detect the proxmark in the client nor the flashing utility.

Last edited by atrox (2010-02-19 21:49:47)

iZsh

Contributor

Registered: 2010-01-02

Posts: 95

Re: Questions about code cleaning

I'm currently trying to install libusb in my mingw environment to see what's going on.

atrox

Contributor

Registered: 2010-01-08

Posts: 35

Re: Questions about code cleaning

ok, the final step to make usb work, is to rename/remove include/usb.h - its simply the w32 version and not compatible with linux. allthough the insertion of usb.h is disputed (http://code.google.com/p/proxmark3/source/detail?r=346) - i suggest, simply renaming it to winusb.h for the sake of windows-compatibility until a better solution is found.

it now detects the proxmark3 at the usb port, but a lot of things still aren't working.

so here is my new patch (and don't forget to rename include/usb.h to include/winusb.h):

diff -u -x .elf -x .d -x '*.o' -x .svn -r proxmark3-read-only.348/client/flash.c proxmark3-read-only.348.atrox/client/flash.c
--- proxmark3-read-only.348/client/flash.c  2010-02-19 15:06:21.000000000 +0100
+++ proxmark3-read-only.348.atrox/client/flash.c  2010-02-19 15:19:11.000000000 +0100
@@ -5,6 +5,7 @@
 BOOL UsbConnect(void);
 #else
 #include <proxusb.h>
+#include <unistd.h>
 #endif
 
 #include <usb_cmd.h>
@@ -18,7 +19,11 @@
 
 static uint32_t ExpectedAddr;
 static uint8_t QueuedToSend[256];
+#ifdef WIN32
 static BOOL AllWritten;
+#else
+static bool AllWritten;
+#endif
 #define PHYSICAL_FLASH_START 0x100000
 #define PHYSICAL_FLASH_END   0x200000
 
diff -u -x .elf -x .d -x '*.o' -x .svn -r proxmark3-read-only.348/client/Makefile proxmark3-read-only.348.atrox/client/Makefile
--- proxmark3-read-only.348/client/Makefile  2010-02-19 15:06:21.000000000 +0100
+++ proxmark3-read-only.348.atrox/client/Makefile  2010-02-19 15:56:11.000000000 +0100
@@ -12,8 +12,8 @@
 QTLDLIBS = $(shell pkg-config --libs QtCore QtGui 2>/dev/null)
 
 CMDSRCS = \
-      $(VPATH)\crc16.c \
-      $(VPATH)\iso14443crc.c \
+      $(VPATH)/crc16.c \
+      $(VPATH)/iso14443crc.c \
       data.c \
       graph.c \
       ui.c \
diff -u -x .elf -x .d -x '*.o' -x .svn -r proxmark3-read-only.348/client/proxusb.h proxmark3-read-only.348.atrox/client/proxusb.h
--- proxmark3-read-only.348/client/proxusb.h  2010-02-19 15:06:21.000000000 +0100
+++ proxmark3-read-only.348.atrox/client/proxusb.h  2010-02-19 22:03:09.000000000 +0100
@@ -10,7 +10,11 @@
 #include <stdint.h>
 #include <stdbool.h>
 #endif
-#include "usb.h"
+#ifdef WIN32
+#include "winusb.h"
+#else
+#include <usb.h>
+#endif
 #include "usb_cmd.h"
 
 extern unsigned char return_on_error;

Last edited by atrox (2010-02-19 22:28:46)

d18c7db

Contributor

Registered: 2008-08-19

Posts: 292

Re: Questions about code cleaning

There is no dispute, usb.h shouldn't be there, it's a hack that got the windows client compiling again. It seems without it compile bombs out due to usb_dev_handle being undefined, not sure what happened, possibly something Makefile related, will look into it some more.

iZsh

Contributor

Registered: 2010-01-02

Posts: 95

Re: Questions about code cleaning

I'm working on it too. r346 changes will eventually get canceled once I complete it.

iZsh

Contributor

Registered: 2010-01-02

Posts: 95

Re: Questions about code cleaning

I fixed the compilation issues on both *nix and windows. Although the windows changes are far from being final.

app_o1

Contributor

Registered: 2013-06-22

Posts: 247

Re: Questions about code cleaning

Where is the "hisampless" command ? Was it replaced ?
I need it to work with TI tags...