Skip to content

Conversation

@harshilbhatt2001
Copy link
Collaborator

@harshilbhatt2001 harshilbhatt2001 commented Mar 16, 2025

Fix #9

Creates a new file with a .D extension containing the following data per audio sample.

typedef struct __attribute__((__packed__)) {
	uint32_t seconds;
	uint16_t milliseconds;
	uint16_t buffer_size;
	uint8_t is_mono;
	uint8_t padding;
} audio_metadata_t;

Copy link
Contributor

@Era-Dorta Era-Dorta left a comment

Choose a reason for hiding this comment

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

Great start!

Copy link
Contributor

@Era-Dorta Era-Dorta left a comment

Choose a reason for hiding this comment

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

Neat!

@Era-Dorta Era-Dorta requested a review from Josfemova March 25, 2025 15:51
Copy link
Contributor

@Era-Dorta Era-Dorta left a comment

Choose a reason for hiding this comment

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

Sorry, a few more questions. This is a very sensitive change, so I want to make sure that we get it right.

@Era-Dorta
Copy link
Contributor

@Josfemova , can you please also review this PR?

Comment on lines +113 to +115
const uint32_t base_sample_rate = 20000; // 20kHz
uint64_t current_time = systick_get_millis();
uint32_t buffer_duration = (PDM_BUF_SIZE / base_sample_rate) * 1000;
Copy link
Collaborator

Choose a reason for hiding this comment

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

sample rate can be a compile-time constant, and the result from the operation PDM_BUF_SIZE / base_sample_rate) * 1000 could be too due to const evaluation if this calc was in a macro instead.

Not functionally different but I would expect that to yield better performance by a slight margin.
Perhaps the calculation of these can be combined with this calculation: https://docs.nordicsemi.com/bundle/ps_nrf5340/page/pdm.html
you could move the value for pdfm_cfg.clock_freq in drv_audio_pdm.c to a define.

You could also take advantage that this now supports C23 and constexpr can be used

if (current_time >= buffer_duration) {
timestamp_buffer[l] = current_time - buffer_duration;
} else {
timestamp_buffer[l] = current_time;
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure why the false case is needed here. If this is a second entry the current_time would be by definition at least 2 buffers duration's in value wouldn't it? Could you elaborate, perhaps I'm missing something

Copy link
Collaborator

Choose a reason for hiding this comment

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

  • @Era-Dorta This got me thinking, wouldn't it be nice to have an endpoint to tell a midge what the current time is? to have a reference to compare the timestamps against?
    IDK if you would consider that useful but came to mind while reviewing this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could you elaborate

It's for potential timer wrap-arounds. If the timer was about 0xFFFFFFF0and it wraps around to 0x0 then the initial condition will be false and you'd get nonsense values. It shouldn't really happen though because the midge won't be running that long.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That was the main reason I initially thought but in practical terms, a Midge will never be ON for enough time for a 64bit msec value to overflow. I think we can safely assume the wrap-around case will never be executed

Copy link
Collaborator

@Josfemova Josfemova left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@Era-Dorta Era-Dorta left a comment

Choose a reason for hiding this comment

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

lgtm too, I had one comment that I forgot to click send on but feel free to merge as is.

Copy link
Contributor

@Era-Dorta Era-Dorta left a comment

Choose a reason for hiding this comment

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

Answered in the comment, all good to merge.

@harshilbhatt2001 harshilbhatt2001 merged commit 135cac1 into master May 14, 2025
@Era-Dorta Era-Dorta deleted the feat/audio_timestamp branch May 15, 2025 07:41
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.

Missing time stamps for the audio

4 participants