search

fpga-open-speech-tools / simulink_models

Simulink models for speech and audio processing
MIT License
4 stars 4 forks source link

Merge simple Matlab DPR example project #51

Closed fe-tdavis closed 3 years ago

tvannoy commented 3 years ago

Just for my edification, does this model support initializing the RAM in the bitstream, or does it need to written to from Linux to have any data? I recall that the primary thing that @BaileyGalacci and @JustinWilliamsMSU did with their DPRAM blocks in https://github.com/fpga-open-speech-tools/simulink_library was to enable the DPRAM to be initialized in the Simulink model itself.

tvannoy commented 3 years ago

I didn't look at the model, but I think this looks good overall. A short README would be helpful, as it would answer some of the questions I've had so far (e.g. what this model does and how to use/modify it).

fe-tdavis commented 3 years ago

support initializing the RAM

I haven't tried initializing the RAM with the bitstream specifically with this model. The main purpose of this (initial) project was to create a prototype interface for loading values into the MATLAB DPR. Maybe in the future we can add more to it! It'll get a README eventually as well.

tvannoy commented 3 years ago

support initializing the RAM

I haven't tried initializing the RAM with the bitstream specifically with this model. The main purpose of this (initial) project was to create a prototype interface for loading values into the MATLAB DPR. Maybe in the future we can add more to it!

I guess my question is do we need support for initializing the DPRAM? Because if so, we have that functionality in a custom Simulink block.

It'll get a README eventually as well.

Unless this is truly just a throwaway project (in which case, it probably doesn't even need to be merged into dev), I think creating a README now (it shouldn't take long) is better than doing it at some undetermined point in the future. If our previous work is any indicator, documentation doesn't often get created after the initial work on the feature is done.

dack-fe commented 3 years ago

I support READMEs

fe-tdavis commented 3 years ago

I think creating a README now

There's already a README being written, although I could see where "eventually" would seem to imply it wouldn't be finished for a while...

tvannoy commented 3 years ago

I think creating a README now

There's already a README being written, although I could see where "eventually" would seem to imply it wouldn't be finished for a while...

:+1: My main point was that I think we should strive to have READMEs before merging PRs.

fe-tdavis commented 3 years ago

👍 My main point was that I think we should strive to have READMEs before merging PRs.

I agree it's a good goal, but not always feasible.

tvannoy commented 3 years ago

+1 My main point was that I think we should strive to have READMEs before merging PRs.

I agree it's a good goal, but not always feasible.

The question is is it feasible in this PR?

fe-tdavis commented 3 years ago

The question is is it feasible in this PR?

As I said, the README is currently being written