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

CCSMB-5: PCM Audio Transmissions over modem #18

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

EmmaKnijn
Copy link
Member

Here is CCSMB-5, suggestions are welcome!

@EmmaKnijn EmmaKnijn added the classification: proposal Introduction of a new proposal. label Jan 7, 2023
Standards/CCSMB-5.md Outdated Show resolved Hide resolved
Standards/CCSMB-5.md Outdated Show resolved Hide resolved

## Packet
A packet consists of the following fields in a table:
- buffer: Signed 8-bit PCM @ 48kHz
Copy link
Member

Choose a reason for hiding this comment

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

Might want to elaborate on this later on.

Copy link
Member Author

Choose a reason for hiding this comment

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

znepb suggested that that might be a standard as well

Standards/CCSMB-5.md Outdated Show resolved Hide resolved
@tomodachi94

This comment was marked as resolved.

@MasonGulu
Copy link

There should be a field that uniquely identifies a packet as following this spec. With the addition of that, the field names could be simplified into station, and title.

@EmmaKnijn

This comment was marked as resolved.

@MasonGulu

This comment was marked as resolved.

@EmmaKnijn

This comment was marked as resolved.

Standards/CCSMB-5.md Outdated Show resolved Hide resolved
@tomodachi94
Copy link
Member

Should we make clients reject packets from servers with an invalid protocol field in order to be compliant?

@EmmaKnijn
Copy link
Member Author

EmmaKnijn commented Jan 7, 2023

Should we make clients reject packets from servers with an invalid protocol field in order to be compliant?

It would create more stability, I'm expecting to expand this someday in another standard so that should be good

@EmmaKnijn
Copy link
Member Author

I think we're ready right?

@Erb3

This comment was marked as off-topic.

@MasonGulu
Copy link

Wait for more feedback.

Erb3
Erb3 previously requested changes Jan 7, 2023
Standards/CCSMB-5.md Outdated Show resolved Hide resolved
Standards/CCSMB-5.md Outdated Show resolved Hide resolved
Standards/CCSMB-5.md Outdated Show resolved Hide resolved
EmmaKnijn and others added 2 commits January 7, 2023 11:37
Co-authored-by: Erlend <erlend.bergersen@sandnesskolen.no>
Copy link
Contributor

@MCJack123 MCJack123 left a comment

Choose a reason for hiding this comment

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

This feels like a very rough draft of a proper standard. Maybe I'm just picky, but standards should clearly explain every relevant part of the standard, and define strict rules where implementors can quickly determine whether they match or not. There's still a lot of ambiguity here, and as-is describes more of a loose data format to follow instead of a proper end-to-end protocol.

One thing I'm a bit stuck on in general is channel choice - having limited channel choice is a bit weird when we have Rednet able to not only specify (theoretically) infinite server endpoints, but it also doesn't have to worry about channel collisions, as the ID of the computer uniquely identifies the source of the data. A drawback is that by using Rednet, every station in the world will always be sending Rednet messages to every computer using Rednet, which isn't quite desirable. An alternative may be to use the current method, but with the computer ID to identify the server instead.

As for discovery, I propose a broadcast message to query what stations are available/in range. This can work like GPS, where a message is sent on a standardized channel, and all servers reply with their metadata, including channel number (in the reply channel field?). This would enhance this otherwise simple protocol with more user-friendly features. It would also allow users to use the server name to connect, instead of having to remember the specific channel number (imagine having to remember an IP instead of domain name for every site); as well as the ability to have full channel listings and browsers.


I think we're ready right?

Standards should be in the queue for at least a week for all kinks to be worked out by as many people as possible. RFCs take years to pass to make sure that there's been plenty of consensus on every exact detail.

Standards/CCSMB-5.md Show resolved Hide resolved
Standards/CCSMB-5.md Outdated Show resolved Hide resolved
Standards/CCSMB-5.md Outdated Show resolved Hide resolved
- title: A string that contains the song that is currently being played
- protocol: A string that contains the protocol used

In order to be compliant the
Copy link
Contributor

Choose a reason for hiding this comment

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

This is worded a bit informally; I'd prefer it to use full sentences in bullet points or an ordered list.

We should also mention RFC 2119 if using keywords like "MUST", "SHOULD", etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you write an example, I feel like writing the same In order to be compliant the in 5 bullet points is a bit redundant

Standards/CCSMB-5.md Outdated Show resolved Hide resolved
Standards/CCSMB-5.md Outdated Show resolved Hide resolved
Standards/CCSMB-5.md Outdated Show resolved Hide resolved
Standards/CCSMB-5.md Outdated Show resolved Hide resolved
Standards/CCSMB-5.md Outdated Show resolved Hide resolved
Standards/CCSMB-5.md Outdated Show resolved Hide resolved
@walksanatora

This comment was marked as resolved.

@EmmaKnijn

This comment was marked as resolved.

@EmmaKnijn

This comment was marked as resolved.

@EmmaKnijn
Copy link
Member Author

Some code examples would be nice too, especially basic clients.

I'll work on that once it's pretty much set in stone

It also needs some language improvement to have a professional tone.

I agree

@EmmaKnijn
Copy link
Member Author

My biggest concern with this is that there is no validation whatsoever. Anyone could easily pretend to be the radio station and send whatever they want.

I honestly don't know if this is fixable over CC, if you compare it to real life, anyone with a Pi can transmit over FM

@EmmaKnijn
Copy link
Member Author

and personally the thing i dont like is it does not contains what happens if a server were to transmit early and what if a client joins mid-song, is the client expected to wait or ... i propose a packet ID system packets count UP, thatway clients can keep watch for the "next packet" and if it recieves packet 0 then we know that the next song has startd and we can reset the counter to 0 and also a server-bound packet to request re-send latest audio packet (in the case a client connects mid-song)

Packets are up to 2.7s long, It would take like 6 seconds at max to get a solid connection

@tomodachi94
Copy link
Member

@EmmaKnijn @MCJack123 what if stations broadcasted some information like station name, channel number etc over a specified channel every, say, 15 seconds, and had a specific range of channel IDs that are specified for radio but aren't hard coded?

@MCJack123
Copy link
Contributor

what if stations broadcasted some information like station name, channel number etc over a specified channel every, say, 15 seconds, and had a specific range of channel IDs that are specified for radio but aren't hard coded?

I'd rather make it be a request-response system - the client sends a request for servers on a standardized channel, and all servers respond with their names/channel number. Broadcasting to nobody is just wasted CPU time.

Standards/CCSMB-5.md Outdated Show resolved Hide resolved
@Erb3
Copy link
Contributor

Erb3 commented Feb 28, 2023

You have a lot of feedback to work with here, and a fair bit of work to do. I would also request that do not make tons of GitHub comments for responding, when you can quote multiple messages in one message.

cc @EmmaKnijn

bilde


I am all for the idea for server discovery. A client sends a message on a specific channel, and all stations reply on that channel with names / channel.


I am not sure how I feel about encryption or signing of the radio. Afaik it would take valuable time to verify this before playing the audio?

@EmmaKnijn
Copy link
Member Author

Added a bunch of things, feel free to have another look

@Erb3
Copy link
Contributor

Erb3 commented Mar 13, 2023

Still a lot of unresolved conversations here. Not ready for merge.

@EmmaKnijn
Copy link
Member Author

Alright, I iterated again. Feel free to have another look.

@EmmaKnijn EmmaKnijn dismissed Erb3’s stale review January 10, 2024 07:15

erb3 has left CCSMB

@tomodachi94 tomodachi94 added the status: stale An unmerged pull request has sat for a month with no comments. label Mar 24, 2024
@EmmaKnijn EmmaKnijn changed the title CCSMB-5: Radio Transmissions CCSMB-5: PCM Audio Transmissions over modem Mar 28, 2024
@EmmaKnijn EmmaKnijn added status: reviews needed A proposal needs reviews. and removed status: stale An unmerged pull request has sat for a month with no comments. labels Mar 30, 2024

## Packet
An audio packet consists of the following fields in a table:
- `buffer`: A table that contains up to 8 tables of signed 8-bit PCM @ 48kHz audio
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we index this as
buffer.fr
buffer.fl
buffer.fc
etc...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
classification: proposal Introduction of a new proposal. status: reviews needed A proposal needs reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants