Skip to content

Conversation

@ellisgl
Copy link
Contributor

@ellisgl ellisgl commented Jul 24, 2025

Figured a C/C++ TCP server that works with Hackrf would be nice.

@Theldus Theldus requested review from Theldus and rlaneth July 27, 2025 01:29
@Theldus
Copy link
Owner

Theldus commented Jul 27, 2025

Hi @ellisgl,
Thank you for the tool for HackRF, seems pretty neat.
As I don't have a HackRF, I can't test it myself, but regarding the code I have some comments.

return 1;
}

int PORT = config["PORT"];
Copy link
Owner

Choose a reason for hiding this comment

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

Although I agree with the appeal of json, it bothers me a bit to bring a 25k LOC library into the repo for just 6 paramaters... maybe they could be environment variables?

Having a single vars.sh like:

PORT=
SAMPLE_RATE=
BITRATE=
AMPLITUDE=
FREQ_DEV
TX_GAIN=

so the user can just 'source it': . vars.sh before using the tool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll look into that

*
* @return std::vector<double> A vector of doubles representing the FSK modulated signal.
*/
std::vector<double> generate_fsk_signal(
Copy link
Owner

Choose a reason for hiding this comment

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

I didn't see this function being used anywhere, please consider using it or removing it entirely.

Comment on lines 123 to 134
// Define the type of socket address, in this case IPv4
address.sin_family = AF_INET;
// Allow connections from any IP address
address.sin_addr.s_addr = INADDR_ANY;
// Set the port number
address.sin_port = htons(PORT);

// Bind the socket to the address and port
if (bind(server_fd, (struct sockaddr *)&address, sizeof(address)) < 0) {
perror("bind failed");
return 1;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed the server currently binds to all IPv4 addresses by default.

Since this looks like it's intended as a simple server without built-in authentication, it might be safer to default to localhost only. This would prevent accidental exposure on network interfaces.

Please consider:

  • Default to binding localhost (127.0.0.1)
  • Add a configuration option so users can explicitly choose to bind to other network interfaces when needed

A BIND_ADDRESS environment variable could work.

Comment on lines 175 to 176
int capcode = std::stoi(capcode_str);
int frequency = std::stoi(freq_str);
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a stability issue with the input parsing that might be worth addressing.

The server currently uses std::stoi() to parse capcode and frequency values from network input:

int capcode = std::stoi(capcode_str);     // Line 175
int frequency = std::stoi(freq_str);      // Line 176

Since std::stoi() throws exceptions on invalid input (like non-numeric strings or values outside int range), any malformed data will currently crash the server. This could happen accidentally with corrupted data or if someone sends unexpected input.

Quick test case:
If you connect via nc localhost 16175 and send something like 123|test message|not_a_number, the server will terminate with an unhandled exception.

Possible fix:
We could wrap these conversions in try-catch blocks to handle bad input gracefully:

try {
    int capcode = std::stoi(capcode_str);
    int frequency = std::stoi(freq_str);
    // ... continue processing
} catch (const std::exception& e) {
    printf("Skipping invalid input: %s\n", e.what());
    close(client_fd);
    continue;  // Handle next client/input
}

This way the server stays running even when it receives unexpected data. What do you think about this approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I'm working on breaking things down. I figured having all the stuff just shoved in main is kind of sloppy. I will admit to being lazy and letting the nice AI overlords take one for the team. heh.

@geekinsanemx
Copy link
Contributor

geekinsanemx commented Jul 30, 2025

Hi Guys!, hope everyone is doing great !, I'm following the thread, and I have a HackRF One to test with, I tried to compile and run the code, but faced some errors, I already fixed in the code the errors, and tested with a real hackrf and advisor elite,

but wanted ask first if is okay to push my changes here?, don't want to disturb anybody

@ellisgl
Copy link
Contributor Author

ellisgl commented Jul 30, 2025

Hey @geekinsanemx
I was going to start over from scratch for the most part. If you want to take this on, go right ahead. =)

Hi Guys!, hope everyone is doing great !, I'm following the thread, and I have a HackRF One to test with, I tried to compile and run the code, but faced some errors, I already fixed in the code the errors, and tested with a real hackrf and advisor elite,

but wanted ask first if is okay to push my changes here?, don't want to disturb anybody

@geekinsanemx
Copy link
Contributor

Thank you so much @ellisgl unable to update over here, but pushed a PR at your fork: ellisgl#2

@Theldus
Copy link
Owner

Theldus commented Aug 2, 2025

Hi guys,
Thank you very much for the teamwork in reviewing the points raised by me and @rlaneth.

@geekinsanemx thanks for polishing @ellisgl’s work and addressing the comments I made =).

The only thing that bothered me a bit was that changes were also made to the GNURadio demo. I'm not sure if that was accidental or not, but I would have preferred that to be in a separate PR if possible. Still, I can go ahead and merge this as-is, excluding those changes.

@rlaneth any additional thoughts on this PR? It looks good to me now.

@geekinsanemx
Copy link
Contributor

geekinsanemx commented Aug 2, 2025

Hi @Theldus !!

ohhh yeap, that's because when I tested with my hackrf find out that I'm using osmosdr_sink, don't know why soapy_hackrf_sink does not work for me, tried to configure hackrf=0 as device argument but unable to send message, I though was more like a simulated sink, that's why changed for osmosdr_sink to actually be able to transmit messages

reverting back file to original and adding osmosdr_sink too :)

if you exclude those gnuradio changes at merging this, I can open a new PR for adding the osmosdr_sink if you wish! :)

Copy link
Collaborator

@rlaneth rlaneth left a comment

Choose a reason for hiding this comment

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

Hi everyone,

I agree that modifying hackrf_file_tx_demo.grc is probably not in the scope of this PR. There's a few concerns that have been raised about this flowchart by other users (as in Issue #7), so it might be due to changes in the near future anyway.

As for the TCP server itself, I see no more issues! I have also tested basic functionality with my own HackRF and pagers, and it works flawlessly.

LGTM

reverting back soapy_hackrf_sink, but adding osmosdr_sink
@Theldus Theldus merged commit 702f14d into Theldus:master Aug 2, 2025
1 check passed
@Theldus
Copy link
Owner

Theldus commented Aug 2, 2025

Thank you guys for the excellent work, merged in 702f14d.

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.

4 participants