Skip to content

Fix bug due to overwriting of asrc_io->input_timestamp #125

Merged
shuchitak merged 11 commits intoxmos:developfrom
shuchitak:save_producer_timestamp
Jun 26, 2024
Merged

Fix bug due to overwriting of asrc_io->input_timestamp #125
shuchitak merged 11 commits intoxmos:developfrom
shuchitak:save_producer_timestamp

Conversation

@shuchitak
Copy link
Copy Markdown
Contributor

@shuchitak shuchitak commented Jun 21, 2024

  • Double buffer the producer timestamp to prevent it getting overwritten during asrc processing.
  • Cleanup 'xscope_used' flag based xscope probe output. Probing enabled only with the ASYNC_FIFO_XSCOPE_INSTRUMENTATION define now.

Comment thread lib_src/src/asrc_task/asrc_task.c Outdated
while(1){
// Wait for block of samples. We will get the buffer index of the newly written samples from receive_asrc_input_samples_cb
unsigned input_write_idx = (unsigned)chanend_in_byte(c_buff_idx);
int32_t save_timestamp = asrc_io->input_timestamp;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

HI, as discussed I think this should be double buffered in the asrc_io struct ideally. Although arguably this is all hidden from the user.
You did mention something about an issue with that approach?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hey @ed-xmos, no issue as such. I'd mentioned that doing it the double buffer way would require more lines of code change since we're changing the asrc_in_out_t struct. It'll require change in lib_src as well as any applications that use it (an02003 for example). I do agree that that's a better fix so will do it the double buffer way and make the change everywhere.

@shuchitak shuchitak requested a review from ed-xmos June 25, 2024 15:09
Comment thread CHANGELOG.rst Outdated
2.6.0
-----

* CHANGED: Double buffer asrc_io.input_timestamp to prevent producer timestamp
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably a fix rather than a change?

Copy link
Copy Markdown
Contributor

@xross xross Jun 25, 2024

Choose a reason for hiding this comment

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

Also, looks like an API change to me (xscope_used)

Copy link
Copy Markdown
Contributor

@ed-xmos ed-xmos left a comment

Choose a reason for hiding this comment

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

Looks great - only suggested change is the changelog entry type

@shuchitak shuchitak merged commit e09a34c into xmos:develop Jun 26, 2024
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.

3 participants