-
Notifications
You must be signed in to change notification settings - Fork 7
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
Acoustic event detector #46
base: master
Are you sure you want to change the base?
Conversation
APP_DIR := $(dir $(abspath $(lastword $(MAKEFILE_LIST)))) | ||
|
||
# files needed for this code | ||
C_SRCS := $(wildcard *.c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the Makefile variable you want to adjust if you want to have a non-flat hierarchy. I believe something like this would include all C files in all subdirectories work:
C_SRCS := $(wildcard *.c)
C_SRCS += $(wildcard **/*.c)
You could also be more explicit if you only want to include certain subdirectories or files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Amit,
Thank you so much for your help! I tried
C_SRCS := $(wildcard *.c)
C_SRCS := $(wildcard **/log2fix.c)
but still get
DEP log2fix/log2fix.c
<built-in>: fatal error: opening dependency file /mnt/c/Users/Long/Projects/signpost/software/apps/audio_module/event_detector/build/cortex-m4/log2fix/log2fix.d: No such file or directory
compilation terminated.
Namely, the Makefile still looks under the build/
but didn't copy the source files into it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh... yes you are right... the problem is that there is no lib2fix
subdirectory in the build/cortex-m4
directory and our Makefile rule assumes there is (and GCC won't just create one). If you create one after the build fails the first time, it will be able to move past it, but that's not solution.
@ppannuto please halp! You are the resident dependency file expert!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick follow-up on this motivated by today's phone call. This should be largely fixed by the updates to the Tock build system. I updated signpost a few days ago in the tock-master
branch, but we're holding off on merging that until after the paper deadline. I'll follow up again on April 11 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Pat,
Thanks for the follow-up. And no worries at all, I understand the burden of a paper deadline. We'll get this working soon.
In the meantime, godspeed to your paper!
@@ -0,0 +1,14 @@ | |||
# Fixed-point ridge tracker for acoustic event detection in C |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this README is just copied from the original repo, and maybe with proper modifications to the Makefile, that's just a submodule or something. However, I think it would be good to have a signpost-port-specific README that
-
explains a bit more about what the app actually does (e.g. "Detects acoustic events", what "acoustic events" are, etc),
-
and gives instructions relevant to Signpost (e.g. you can't run
main
with arguments on Signposts).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your guess is correct! I've updated the README according to your suggestion. Thank you.
Use test.ipynb to visualize the resulting \*.mat files | ||
|
||
## Prerequisites | ||
* Fixed-point FFT: git clone https://github.com/longle2718/kiss_fft |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these dependecies are vendored (which I believe you've done) I don't think they need to be listed as prerequisites.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. However, once we have a hierarchical directory structure, we won't be copying the sources and just leave them as prerequisites. Thanks!
a->used = 0; | ||
|
||
a->size = a->maxSize; | ||
printf("Init array size = %zu\n",a->size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a debug message? Perhaps better for library code to do this in a DEBUG_PRINTF
macro that can be turned off at compiled time? (As opposed to printfs from the actual application code, like in main.c
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for telling me about DEBUG_PRINTF! This will be fixed in the next pull request.
return 0; | ||
} | ||
|
||
static int freqAnalyze(kiss_fft_scalar *frame, kiss_fft_scalar *spec){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, these static helper methods are returning positive integers on error, which is pretty unintuitive for C. I think it would be better to return negative numbers (common for errors) and check if(helper_function() < 0)
in main
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thank you very much for telling me about the correct style in C. I'll fix this accordingly.
} | ||
|
||
static int printMat(char *filename, kiss_fft_scalar *in, size_t N){ | ||
FILE *fp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So... There is no support currently for storing files. In principal, we could implement a filesystem that ships over to the SD card on the main board, but currently I believe it will always fail on fopen
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's what I suspected, too. I plan to replace these printMat
with the appropriate Signpost's memory API to log results. We'll definitely need to ship events out of the Signpost for more processing.
This is cool! I guess the goal is to eventually either ship events out over the radio or use this as a library for a larger application that uses the events as a signal to start doing other processing? |
Hi Amit, Thank you so much for your time and patience with my first pull request! I really appreciated your guidance. Yes, my current goal is to ship events out of the Signpost for more processing. In my experience on the Android platform, the right way to do this is to write detected events down to a local storage queue, and depends on a "Network/Communication Manager" to clear the queue/ship events off the board depending on the network state/energy availability. Thoughts? The event detector you see is indeed a subroutine/library in a larger processing chain. ;) |
Hi guys,
This is our universal acoustic event detector. Correct operation still depends on the ADC driver though.
Currently, all source files are copied-and-pasted from another debugging repos (https://bitbucket.org/longle1/gcwa) and laid out flat to build with the default Makefile. I could use some help with the Makefile to reorganize the code and avoid copy-and-paste everytime there's an update in the original repo. :)
Thanks,
Long