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

Documents improvements #347

Merged
merged 10 commits into from
Jul 20, 2024
Merged

Conversation

TheDaChicken
Copy link
Contributor

@TheDaChicken TheDaChicken commented Jun 25, 2024

Deals with #346

It wasn't straightforward what the sample rate was meant to be for either of the 2 pipe functions. This adds macro and documents it for each function.

This also deals with a warning that Clion IDE gave me. It warned me that __declspec(dllexport) was meant to be used in the header. If needed, this part can be removed. This is a nick-pick. CMake newer versions can do this automatically using set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON) though is not supported with this 2.8.

If you think of other constants that should be added, let me know.

My program has a simple #define NRSC5_MPS_PROGRAM 0x0 constant. Not sure if that is worth adding. It is simple zero.

A) Added sample rate constants
B) Added doc for seeing sample rate constants
C) Move NRSC5_API to header
support/nrsc5.py Outdated Show resolved Hide resolved
@argilo
Copy link
Collaborator

argilo commented Jul 19, 2024

This also deals with a warning that Clion IDE gave me. It warned me that __declspec(dllexport) was meant to be used in the header. If needed, this part can be removed.

If you don't mind, I'd rather consider that in a separate PR.

@TheDaChicken
Copy link
Contributor Author

TheDaChicken commented Jul 19, 2024

This also deals with a warning that Clion IDE gave me. It warned me that __declspec(dllexport) was meant to be used in the header. If needed, this part can be removed.

If you don't mind, I'd rather consider that in a separate PR.

Alright, I created, #350!

src/defines.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@argilo argilo left a comment

Choose a reason for hiding this comment

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

Looks good now. I'll merge once CI is happy.

@TheDaChicken
Copy link
Contributor Author

Also a few things before merge:

Are you sure that it decimates only 2 from cs16? What about AM? There is DECIMATION_FACTOR_AM in acquire.c and also looks like it decimates by 32 in nrsc5.c

Should there be an NRSC5_PACKET_SAMPLES constant to prevent the 2048 magic number in main.c?

@argilo
Copy link
Collaborator

argilo commented Jul 19, 2024

Are you sure that it decimates only 2 from cs16? What about AM? There is DECIMATION_FACTOR_AM in acquire.c and also looks like it decimates by 32 in nrsc5.c

That's a good point. For AM, there are five cascaded decimators, so the CS16 sample rate is 1488375 / 32 = 46511.71875. It would make sense to have separate constants for FM and AM.

Should there be an NRSC5_PACKET_SAMPLES constant to prevent the 2048 magic number in main.c?

Yes, I think that would be nice. They are called audio frames in the specification, so perhaps the name could be NRSC5_AUDIO_FRAME_SAMPLES.

@TheDaChicken
Copy link
Contributor Author

Are you sure that it decimates only 2 from cs16? What about AM? There is DECIMATION_FACTOR_AM in acquire.c and also looks like it decimates by 32 in nrsc5.c

That's a good point. For AM, there are five cascaded decimators, so the CS16 sample rate is 1488375 / 32 = 46511.71875. It would make sense to have separate constants for FM and AM.

Should there be an NRSC5_PACKET_SAMPLES constant to prevent the 2048 magic number in main.c?

Yes, I think that would be nice. They are called audio frames in the specification, so perhaps the name could be NRSC5_AUDIO_FRAME_SAMPLES.

Alright. Those have been added!

@argilo argilo merged commit 2a85627 into theori-io:master Jul 20, 2024
6 checks passed
@TheDaChicken TheDaChicken deleted the doc-improvements branch January 14, 2025 00:34
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.

2 participants