Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Real-time LF sampling #2173

Merged
merged 11 commits into from
Dec 1, 2023
Merged

Conversation

wh201906
Copy link
Contributor

@wh201906 wh201906 commented Nov 14, 2023

#2167 with clear commit history and some minor improvements

Known limitations:

  1. Only bits_per_sample = 8 is supported now.
  2. Only USB connection is supported now. The transfer speed should be greater than 300kByte/s to support this feature.
  3. When LF image is not loaded into the FPGA, the first 14 bytes could be the response of CMD_WTX rather than real samples. (Fixed in Fix corrupted data caused by CMD_WTX #2193)

1280000 samples -> 10.24s for 125kHz 8bit sampling

Allocate graph related memory on heap
Use longer timeout in WaitForRawDataTimeout() to handle CMD_WTX
Fix a wrong type
Apply changes to other similar part
Remove unused instructions
Copy link

You are welcome to add an entry to the CHANGELOG.md as well

@wh201906 wh201906 marked this pull request as ready for review November 14, 2023 02:50
@wh201906 wh201906 mentioned this pull request Nov 14, 2023
@wh201906
Copy link
Contributor Author

@henrygab @iceman1001 Could you please review this PR?

Copy link
Contributor

@henrygab henrygab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed all files EXCEPT client/src/comms.c.
Note that I'm just a random user ... so any comments from Iceman can totally overrule my own. I only noticed a couple bugs (even if one was repeated) ... which is amazing for the size of this PR.

At a minimum, checking for and handling failed memory allocations is critical. The rest of my comments are just fit and finish.

Well done!

@@ -42,7 +42,7 @@ int GetNrzClock(const char *str, bool verbose);
int GetFskClock(const char *str, bool verbose);
bool fskClocks(uint8_t *fc1, uint8_t *fc2, uint8_t *rf1, int *firstClockEdge);

#define MAX_GRAPH_TRACE_LEN (40000 * 8)
#define MAX_GRAPH_TRACE_LEN (40000 * 32)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case others wondered:

The maximum graph trace length being hard-coded at compile time appears to be relied on by at least some code in this system.

See, for example, getFromGraphBuf(), which presumes the buffer's size.

Is this therefore also the limit for obtaining longer traces? (i.e., does this limit long traces on clients?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is the only limit for obtaining longer traces in the code. I got stack overflows when just increasing this number. And that's why I made commit 8b6a274 to allocate related arrays on the heap.

client/src/cmdlfio.c Show resolved Hide resolved
client/src/cmdlfhid.c Show resolved Hide resolved
client/src/cmdlf.c Outdated Show resolved Hide resolved
client/src/cmdlf.c Show resolved Hide resolved
armsrc/lfsampling.c Show resolved Hide resolved
armsrc/lfsampling.c Outdated Show resolved Hide resolved
armsrc/lfsampling.c Show resolved Hide resolved
armsrc/util.c Show resolved Hide resolved
client/src/cmddata.c Outdated Show resolved Hide resolved
Check if memory allocation fails
Fix memory leak
Initialize struct in declaration
Add/Fix some notes
Remove unlikely() in favor of readability
Remove a hard-coded magic number
@iceman1001
Copy link
Collaborator

Gentle reminder that we use calloc instead.

Some external libs still uses malloc which we don't want to fiddle with.

@wh201906
Copy link
Contributor Author

wh201906 commented Nov 17, 2023

I replaced declarations like this
uint8_t bits[XXX] -> malloc()
uint8_t bits[XXX] = {0} -> calloc()

Plus, all of the memory allocated by malloc() is actually initialized immediately by getFromGraphBuf() in commit 8b6a274, so I think it's not necessary to zero-initialize them first.
Anyway, it's up to you. Would you prefer to still use calloc() for them?

@iceman1001
Copy link
Collaborator

I found that using calloc has helped us on different platforms, so yes, that is the prefered way.

@wh201906 wh201906 force-pushed the lf_sniff_clean branch 2 times, most recently from ca771e0 to 706c2d0 Compare November 17, 2023 04:30
Suggested by @iceman1001
Mainly for 8b6a274
Replaced the malloc() in getSamplesFromBufEx()
Added memory allocation result check for getSamplesFromBufEx(),
lf_read_internal(), and lf_sniff()
@wh201906
Copy link
Contributor Author

I've replaced them

@iceman1001
Copy link
Collaborator

Since I travelling, I will not be able to look at this PR until I get back.

@wh201906
Copy link
Contributor Author

No problem, have a good time!

@iceman1001
Copy link
Collaborator

Got time to play little with this PR today.

looking good, it has some nice potential.
I found that the lf read -r didn't quite make sense. Should be smoother. end user shouldn't need to know, it should just work.

The demodulation code lfdemod.c has 512 bit limits, which now can be increased much more.

I get some freezes, indicating there are some memory issues still.

but I like it.

scrolling in the plot window is confusing when its this large..

@wh201906
Copy link
Contributor Author

I found that the lf read -r didn't quite make sense. Should be smoother.

But the real-time stuffs has speed requirements. It's not always working, especially for the wireless connections.

I get some freezes, indicating there are some memory issues still.

You mean it's in lfdemod.c?

@iceman1001
Copy link
Collaborator

I would say it should be automatic..
Default sample size or less, should go the old way, if a larger sample size, it should go the alternative way.
lf read isn't like real time sniffing..

Hold on with changing demodulation functions. Otherwise this PR will be become too large.
Better to change that after we get this one merged

@wh201906
Copy link
Contributor Author

wh201906 commented Nov 29, 2023

OK. Is there anything I need to do for improving this PR?

Default sample size or less, should go the old way, if a larger sample size, it should go the alternative way.

Sounds reasonable

@iceman1001
Copy link
Collaborator

Check for memory leaks.

@wh201906
Copy link
Contributor Author

Any steps to reproduce the memory leak bugs?

@iceman1001
Copy link
Collaborator

do different lf / data operations?

@wh201906
Copy link
Contributor Author

wh201906 commented Nov 30, 2023

I got a segmentation fault when running data modulation over 132000 samples.
It should be fixed now.
There are not too many changes to the demodulation functions, so I included them in this PR.

@iceman1001
Copy link
Collaborator

alright,
doing some more testing, this text should be adapted to match new samples.

"data samples -n 10000"

static int CmdSamples(const char *Cmd) {

    CLIParserContext *ctx;
    CLIParserInit(&ctx, "data samples",
                  "Get raw samples for graph window (GraphBuffer) from device.\n"
                  "If 0, then get whole big buffer from device.",
                  "data samples\n"
                  "data samples -n 10000"
                 );
    void *argtable[] = {
        arg_param_begin,
        arg_int0("n", NULL, "<dec>", "num of samples (512 - 40000)"),
        arg_lit0("v", "verbose", "verbose output"),
        arg_param_end
    };
    ```

@iceman1001
Copy link
Collaborator

Or...
data samples is a old style...

@iceman1001
Copy link
Collaborator

The lf search only uses the old one too...

lf search -u

@wh201906
Copy link
Contributor Author

wh201906 commented Dec 1, 2023

I'm not sure what I should do now...
It looks like the data samples is only for fetching existing data from the device memory, so I guess it has nothing to do with the real-time samples.
As for lf search -u, I just tested lf search -1u to use the samples from the graphbuffer(300000 samples), and it doesn't crash.

@iceman1001
Copy link
Collaborator

One of these days, we should refactor lf search to read once and then send through all the demodulators...

Making sure we don't do multiple reads

@iceman1001 iceman1001 merged commit 17a93a3 into RfidResearchGroup:master Dec 1, 2023
@iceman1001
Copy link
Collaborator

Lets merge,
and see what kind of bugs is coming out.

I have some suggestions for improvements as stated above.

@wh201906 wh201906 deleted the lf_sniff_clean branch December 1, 2023 10:02
@wh201906
Copy link
Contributor Author

wh201906 commented Dec 1, 2023

Thank you @iceman1001 and @henrygab for reviewing this PR and #2167

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants