Implement basic server and test probe #12

goodhoko · 2023-04-20T09:56:02Z

Something to start with. The code should be pretty self-explanatory but see the commit messages for details.

I think the biggest next step is figuring out some decent abstractions for the sound synthesis. (#6) I initially tried to use fundsp but couldn't get over the complexity of its APIs. However, it still may be better suited for the sound effects we'll be trying to implement.

Adding more probes on top of this should be simple. That also gives us some good bits to work on next.

Implement the server as a "composer" crate for a (IMO) better fitting name. In addition to the binary give it a library section that exposes logic reused by probes as well as the possible event types. Start with very basic sound effect of just playing a sample from a file. This needs some improvements. But we gotta start somewhere. Also implement a basic test probe that sends test events in a pulsating frequency. closes #9 closes #4

strohel

Nice! I've got a just a bunch of technicalities.

Cargo.toml

crates/composer/Cargo.toml

strohel · 2023-04-20T10:12:51Z

crates/composer/Cargo.toml

+bincode = "1.3.3"
+color-eyre = "0.6.2"
+eyre = "0.6.8"
+rodio = { version = "0.17.1", features = ["symphonia-wav"] }
+serde = { version = "1.0.160", features = ["serde_derive"] }


Somewhat philosophical: cargo follows its own flavour of semver rules, so specifying bincode = "1.3.3" is equivalent to ^1.3.3, which pulls 1.3.x where x >= 3. So if we don't specifically need the patch version to be at least 3, we can as well say

bincode = "1.3"

That gives cargo dependency resolver a bit more leeway to deduplicate dependencies in some corner cases (think of some transitive dependency depending on bincode = "=1.3.2" or similar.

The downside is that it is more work to do this "properly" - if bincode 1.3.1 introdice a (backwards-compatible) feature that we use, we shoudl depend on 1.3.1 and not 1.3.

In tonari codebase, we typically do:

bincode = "1" color-eyre = "0.6" eyre = "0.6"

Only specify the major version past 1.0, and specify minor version before 1.0. This will give cargo the most flexibility assuming that our dependent crates follow the semvar compatibility requirements strictly. @strohel should we do that here as well?

@skywhale yeah that's basically what I intended to say (it convoluted way) in my above comment.

The only exception/caveat is that patch (x.y.Z+1) releases can introduce new features (if they're backwards compatible), and we can depend on the new features. So sometimes specifying all 3 components for dependencies is needed/correct.

I was lazy here and just rolled with whatever cargo add ... produced.

Trying to not block this PR further, in 80d2029 I changed the versions to what @skywhale suggested.

I'll try to get to the philosophical aspect later. Now, I need to go out with the dog. .)

Getting back to the caveat @strohel mentions: I think that there is little reason to aim for the correct (i.e. minimal functional lower bound) version range from the get go. We'd have to essentially enumerate all the "features" we use and backtrack them to the minor version they were all included in. That'd be hard.

Also remember, that there's cargo.lock. If someone adds a dependency = "1.5" and makes our program work but in fact relies on a feature from 1.5.5 then the exact version pinned in cargo.lock must be 1.5.5+. I.e. the caveat can only arise when cargo.lock changes which isn't as often and it should be fairly easy to spot what happened in that case. At least as long as the version mismatch manifests visibly - which is something I'd expect in a language typed as strongly as rust.

@goodhoko agreed. Perhaps libraries (which do not ship Cargo.lock) that are popular should do this, but indeed that seems like an overkill for us.

crates/composer/Cargo.toml

crates/composer/src/lib.rs

crates/composer/src/main.rs

crates/test_probe/src/main.rs

Co-authored-by: Matěj Laitl <[email protected]>

skywhale

Looking great :) Only minor comments.

skywhale · 2023-04-21T07:14:52Z

crates/composer/Cargo.toml

+bincode = "1.3.3"
+color-eyre = "0.6.2"
+eyre = "0.6.8"
+rodio = { version = "0.17.1", features = ["symphonia-wav"] }
+serde = { version = "1.0.160", features = ["serde_derive"] }


In tonari codebase, we typically do:

bincode = "1" color-eyre = "0.6" eyre = "0.6"

Only specify the major version past 1.0, and specify minor version before 1.0. This will give cargo the most flexibility assuming that our dependent crates follow the semvar compatibility requirements strictly. @strohel should we do that here as well?

crates/test_probe/src/main.rs

crates/composer/src/main.rs

crates/composer/src/api.rs

PabloMansanet · 2023-04-21T07:49:48Z

crates/composer/src/main.rs

+    loop {
+        if let Err(err) = {
+            // Should be more than enough.
+            let mut buf = [0; 1000];
+            let (number_of_bytes, _) = socket.recv_from(&mut buf)?;
+
+            let event: Event = bincode::deserialize(&buf)?;
+            println!("Received an event ({number_of_bytes} bytes): {:?}", event);
+
+            // FIXME: do the decoding and file reading outside the loop
+            let file = BufReader::new(File::open("src/sound_samples/click.wav")?);
+            let source = Decoder::new(file)?;
+            stream_handle.play_raw(source.convert_samples())?;
+
+            Ok::<(), eyre::Error>(())
+        } {
+            eprintln!("Could not process data-gram: {:?}", err);
+            continue;
+        }
+    }


I may be completely misreading this (in which case I blame pre-coffee Friday brain) but I think this control flow is not right. The continue line should be impossible to hit under the current circumstances.

There are many fallible functions inside the if let block, and they're all postfixed with the ? try operator. I feel like the intention was that any of these errors would bubble up to the if let and take control flow to the continue block. However, that's not how it would play out; any line erroring inside the if let block would instead exit the main function and terminate the program.

The only way I can see the continue block being hit is if the previous block evaluated to an Err, which is impossible as line 31 is an inconditional Ok.

A way to achieve what (I think) this function wants to do would be to take the block into a separate function, and use its return value. Then it would be OK to use all these try operators as-is.

@PabloMansanet great catch! I completely missed that.

I think this is written as if try { ... } expressions from rust-lang/rust#31436 where stable - but they are not (and this doesn't contain the critical try keyword).

Good catch!

I think this is written as if try { ... } expressions from rust-lang/rust#31436 where stable

This semantic was exactly my intention. .)

I refactored this into a separate function in 5a37564

PabloMansanet · 2023-04-21T07:56:40Z

crates/composer/src/main.rs

+fn main() -> Result<()> {
+    color_eyre::install()?;
+
+    let socket = UdpSocket::bind(DEFAULT_SERVER_ADDRESS)?;


I'm still unsure about using Udp instead of Tcp for this.

If we were talking about sending audio packets or any other kind of short, time-limited, missable data I would definitely use Udp. However, if I understand where we are in terms of abstraction when it comes to events, an event could signify a single occurrence of something very bad. For example, a single event packet could mean reporting a catastrophic error that would result in a few seconds of screeching (I may be wrong on this though since I don't have the context of your in-person chats so correct me if I am!).

If I'm on the right ballpark about the potential importance of events though, we shouldn't be missing any, and I think the reliability of TCP would work better here.

Right. As agreed personally, we'll continue here with UDP but should look into using reliable alternatives. I filled an issue for that #15

PabloMansanet · 2023-04-21T07:58:04Z

crates/test_probe/src/main.rs

+    color_eyre::install()?;
+
+    // Get server address, if given
+    let args: Vec<String> = env::args().collect();


Nit: not super urgent, but I like using clap for all the argument handling so we get all the bells and whistles like usage strings, --help menu, etc :)

Let's do this in a sperate PR. I filled an issue for myself #16

PabloMansanet

Good start! We can definitely start building on this.

I have a couple nits about iterator, UDP and argument handling, but the main one is addressing the control flow issue on the composer main function. Looking good once we sort that out.

Co-authored-by: Matěj Laitl <[email protected]>

and use `impl ToSocketAddrs` as the argument type for `new`.

strohel

The latest iteration is looking very good! Added some last nits, but consider this green-lighted from me already.

crates/composer/src/main.rs

crates/test_probe/src/main.rs

PabloMansanet · 2023-04-24T10:28:57Z

crates/composer/src/main.rs

+                "Could not process datagram. Ignoring and continuing. {:?}",
+                err
+            );
+            continue;


Minor nit: the continue isn't needed anymore.

removed in 4b30780

PabloMansanet

Looking great! No more concerns on my end 👍

skywhale

LGTM++

goodhoko added 2 commits April 20, 2023 11:24

Git-ignore vscode settings

f3d6747

goodhoko mentioned this pull request Apr 20, 2023

Propose Architecture and Probe <-> Server API #11

Merged

strohel reviewed Apr 20, 2023

View reviewed changes

goodhoko and others added 2 commits April 20, 2023 14:41

Update crates/composer/src/main.rs

aca35c1

Co-authored-by: Matěj Laitl <[email protected]>

make composer and test probe default members

08e701d

strohel mentioned this pull request Apr 20, 2023

PTRACE on rust proof of concept #13

Closed

skywhale reviewed Apr 21, 2023

View reviewed changes

PabloMansanet reviewed Apr 21, 2023

View reviewed changes

PabloMansanet suggested changes Apr 21, 2023

View reviewed changes

goodhoko and others added 4 commits April 21, 2023 10:16

Update crates/composer/Cargo.toml

d6aced0

Co-authored-by: Matěj Laitl <[email protected]>

Update crates/composer/src/main.rs

61e0ff8

Co-authored-by: Matěj Laitl <[email protected]>

Update crates/composer/src/main.rs

da701b8

Co-authored-by: Matěj Laitl <[email protected]>

Update crates/composer/src/api.rs

a44c1ec

Co-authored-by: Matěj Laitl <[email protected]>

This was referenced Apr 21, 2023

Research reliable alternative to UDP #15

Open

Use Clap to parse test probes's arguments #16

Closed

goodhoko added 4 commits April 21, 2023 16:42

Relax dependency versions

80d2029

Provide separate "constructor" for default address

d261261

and use `impl ToSocketAddrs` as the argument type for `new`.

Simplify loop test probe looping

e9b00bc

Fix error handling bug

5a37564

goodhoko force-pushed the scaffolding branch from 338c1c6 to 5a37564 Compare April 21, 2023 16:17

goodhoko mentioned this pull request Apr 21, 2023

Add CI (again 🤦) #17

Merged

strohel mentioned this pull request Apr 22, 2023

Add event timestamping #19

Open

strohel approved these changes Apr 22, 2023

View reviewed changes

crates/composer/src/main.rs Outdated Show resolved Hide resolved

crates/test_probe/src/main.rs Outdated Show resolved Hide resolved

crates/test_probe/src/main.rs Show resolved Hide resolved

crates/test_probe/src/main.rs Show resolved Hide resolved

goodhoko added 2 commits April 23, 2023 09:42

Limit deserialization input to received bytes

25fbe2f

Explain usage of unreachable!()

e3fcf83

PabloMansanet reviewed Apr 24, 2023

View reviewed changes

PabloMansanet approved these changes Apr 24, 2023

View reviewed changes

skywhale approved these changes Apr 24, 2023

View reviewed changes

Remove needless continue

4b30780

goodhoko merged commit 6eb0b35 into main Apr 24, 2023

goodhoko deleted the scaffolding branch April 24, 2023 10:37

goodhoko mentioned this pull request Apr 24, 2023

Scaffold the repository #9

Closed

strohel mentioned this pull request May 31, 2023

Various dependency updates tonarino/innernet#265

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement basic server and test probe #12

Implement basic server and test probe #12

goodhoko commented Apr 20, 2023

strohel left a comment

strohel Apr 20, 2023 •

edited

Loading

skywhale Apr 21, 2023

strohel Apr 21, 2023

goodhoko Apr 21, 2023

goodhoko Apr 23, 2023

strohel Apr 23, 2023

skywhale left a comment

skywhale Apr 21, 2023

PabloMansanet Apr 21, 2023

strohel Apr 21, 2023

goodhoko Apr 21, 2023

PabloMansanet Apr 21, 2023

goodhoko Apr 21, 2023

PabloMansanet Apr 21, 2023

goodhoko Apr 21, 2023

PabloMansanet left a comment

strohel left a comment

PabloMansanet Apr 24, 2023

goodhoko Apr 24, 2023

PabloMansanet left a comment

skywhale left a comment

Implement basic server and test probe #12

Implement basic server and test probe #12

Conversation

goodhoko commented Apr 20, 2023

strohel left a comment

Choose a reason for hiding this comment

strohel Apr 20, 2023 • 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

skywhale left a 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

Choose a reason for hiding this comment

PabloMansanet left a comment

Choose a reason for hiding this comment

strohel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PabloMansanet left a comment

Choose a reason for hiding this comment

skywhale left a comment

Choose a reason for hiding this comment

strohel Apr 20, 2023 •

edited

Loading