search

xmos / lib_dsp

Core digital signal processing function library
Other
22 stars 28 forks source link

Header files shouldn't use "lib_" prefix to match other XMOS libraries #52

Closed larry-xmos closed 8 years ago

ThomasGmeinder commented 8 years ago

I see the point about consistency but have a few remarks: IIRC the prefix was introduced to create a name space and avoid clashes with header files in other code bases. Without the prefix we'd end up with filenames like dsp.h, math.h, matrix.h. math.h would already be a clash. So a using a prefix makes sense. CMSIS uses the arm_ prefix (e.g. arm_math.h) lib_voice uses lib_voice_doa_naive.h

andrewstanfordjason commented 8 years ago

The tests and examples are inconsistent too. It would be really nice if they followed the convention of everything else. It makes things difficult for maintenance of the build and release system.

xross commented 8 years ago

Please don't use lib_voice as a reference, it's yet to be productised or reach version 1. It's still very much in the hands of the technology team.

On 10 Jun 2016, at 08:19, Thomas Gmeinder notifications@github.com<mailto:notifications@github.com> wrote:

I see the point about consistency but have a few remarks: IIRC the prefix was introduced to create a name space and avoid clashes with header files in other code bases. Without the prefix we'd end up with filenames like dsp.h, math.h, matrix.h. math.h would already be a clash. So a using a prefix makes sense. CMSIS uses the arm_ prefix (e.g. arm_math.h) lib_voice uses lib_voice_doa_naive.h

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/xmos/lib_dsp/issues/52#issuecomment-225111097, or mute the threadhttps://github.com/notifications/unsubscribe/AAnPXma5B2U6aJ0Q_qcxc872kq2b2pKrks5qKRBDgaJpZM4IyIMa.

johned0 commented 8 years ago

Comments from Thomas - removing the libdsp prefix will cause other issues (lib_dsp_math.h will be turned into math.h which will clash)

I think Andrews request boils down to: Rip AN00209_xCORE-200_DSPLibrary apart and create a separate appnote for all 10 app applications. If we implemented these requests the effort would be much higher than the benefit. Especially for Andrew's request.

I agree with both points. John

pthedinger commented 8 years ago

The request was to remove the lib_ prefix, leaving it as dsp_math.h, so there should be no conflict.

ThomasGmeinder commented 8 years ago

OK. I will rename to remove lib_.

ThomasGmeinder commented 8 years ago

I realised that changing all header files from libdsp....h to dsp.....h will then cause inconsistency in the library itself with the filenames in the src directory as well as all the functions. All of the above have prefix libdsp What shall we do with those?

It's not a big effort to search/replace "lib_dsp" with "dsp" but any user code to date would have to be changed because all the function names changed.

henkmuller commented 8 years ago

Just a left field observation.

We started with one minor change that required a major version bump (something to do with matrices - not sure how many people use that), but by the sound of it this becomes a rather big set of major inconsistencies that will required everybody to change their code.

If that is the case - is that a necessary evil, or isn't it worth it?

ThomasGmeinder commented 8 years ago

Based on the feedback this seems to be the consensus: Moving to 3.0.0 is an opportunity for radical API changes like changing all function names. Changing prefixes of all functions and source file names from "lib_dsp" to "dsp" would bring consistency. All user code will have to change but we think it's worth it because it increases consistency within the library and across libraries.

Unless somebody shouts I will make it so.

andrewstanfordjason commented 8 years ago

while you are at it could we rename lib_dsp_fft_complex_t -> dsp_complex_t and lib_dsp_fft_complex_short_t -> dsp_complex_short_t

as they don't really belone to ffts. This was my mistake but we might as well fix it in one commit. thanks

henkmuller commented 8 years ago

I urge a rethink

We do not need opportunities to change everything. We need to make sure our customers are happy.

On 15 Jun 2016, at 15:34, Andrew Stanford-Jason notifications@github.com<mailto:notifications@github.com> wrote:

while you are at it could we rename lib_dsp_fft_complex_t -> dsp_complex_t and lib_dsp_fft_complex_short_t -> dsp_complex_short_t

as they don't really belone to ffts. This was my mistake but we might as well fix it in one commit. thanks

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/xmos/lib_dsp/issues/52#issuecomment-226206786, or mute the threadhttps://github.com/notifications/unsubscribe/AAmJB_zNWUndQMpXMbtiChxoaW1tN8qSks5qMA3EgaJpZM4IyIMa.

xross commented 8 years ago

One option is to leave the old API in place along side any new API

It would be good practise to mark as appropriate e.g.

ifdef XC

define xscope_probe(id) _Pragma("warning \"xscope_probe is deprecated, use xscope_char instead\"") xscope_char(id, 0)

else

attribute((deprecated)) static inline void xscope_probe(unsigned char id) { xscope_char(id, 0); }

endif

On 15 Jun 2016, at 15:39, Henk Muller notifications@github.com<mailto:notifications@github.com> wrote:

I urge a rethink

We do not need opportunities to change everything. We need to make sure our customers are happy.

On 15 Jun 2016, at 15:34, Andrew Stanford-Jason notifications@github.com<mailto:notifications@github.commailto:notifications@github.com> wrote:

while you are at it could we rename lib_dsp_fft_complex_t -> dsp_complex_t and lib_dsp_fft_complex_short_t -> dsp_complex_short_t

as they don't really belone to ffts. This was my mistake but we might as well fix it in one commit. thanks

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/xmos/lib_dsp/issues/52#issuecomment-226206786, or mute the threadhttps://github.com/notifications/unsubscribe/AAmJB_zNWUndQMpXMbtiChxoaW1tN8qSks5qMA3EgaJpZM4IyIMa.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/xmos/lib_dsp/issues/52#issuecomment-226207952, or mute the threadhttps://github.com/notifications/unsubscribe/AAnPXlY8A9Y-K46o8cx0-ckVnGkJnwbdks5qMA6sgaJpZM4IyIMa.

pthedinger commented 8 years ago

Making it deprecated is actually more work than just changing the interface. Clearly it would be a requirement for libraries that have a lot of users and a long history. How many users of the library are there? We need to be pragmatic given we have limited resource to spend on this.

xross commented 8 years ago

More work for us, less work for the users - just addressing Henk’s point.

On 15 Jun 2016, at 15:52, Peter Hedinger notifications@github.com<mailto:notifications@github.com> wrote:

Making it deprecated is actually more work than just changing the interface. Clearly it would be a requirement for libraries that have a lot of users and a long history. How many users of the library are there? We need to be pragmatic given we have limited resource to spend on this.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/xmos/lib_dsp/issues/52#issuecomment-226212051, or mute the threadhttps://github.com/notifications/unsubscribe/AAnPXtjEIHBFoCubKM2vUX7-bQSpf_tPks5qMBGQgaJpZM4IyIMa.

pthedinger commented 8 years ago

Having discussed this with Henk and Ross we have come to the conclusion that at the moment it is still ok to make this major change to the API without adding the deprecated step. There are not enough external users of the library to prevent us from making the change. So, Thomas, go ahead and make the change.

Thanks,

Peter

ThomasGmeinder commented 8 years ago

Done in latest Pull Request #53

huwg59 commented 8 years ago

I don’t have access to the lib_dsp repo, so please can someone replace all instances of “accumulater" with “accumulator” in the lib_dsp app note. I think there are six instances

Thanks,

Huw

On 15 Jun 2016, at 16:07, Peter Hedinger notifications@github.com wrote:

Having discussed this with Henk and Ross we have come to the conclusion that at the moment it is still ok to make this major change to the API without adding the deprecated step. There are not enough external users of the library to prevent us from making the change. So, Thomas, go ahead and make the change.

Thanks,

Peter

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

ThomasGmeinder commented 8 years ago

Thanks Huw Fixed 24 instances of “accumulater" and pushed. Commit Fixed typohttps://github.com/xmos/lib_dsp/pull/53/commits/3caa5ea9ccfd9fe53219f1d36d54fde53dab7460 was added to the latest pull request https://github.com/xmos/lib_dsp/pull/53

Cheers, /T

From: huwg59 notifications@github.com<mailto:notifications@github.com> Reply-To: xmos/lib_dsp reply@reply.github.com<mailto:reply@reply.github.com> Date: Friday, 17 June 2016 10:06 To: xmos/lib_dsp lib_dsp@noreply.github.com<mailto:lib_dsp@noreply.github.com> Cc: thomas thomas@xmos.com<mailto:thomas@xmos.com>, Comment comment@noreply.github.com<mailto:comment@noreply.github.com> Subject: Re: [xmos/libdsp] Header files shouldn't use "lib" prefix to match other XMOS libraries (#52)

I don’t have access to the lib_dsp repo, so please can someone replace all instances of “accumulater" with “accumulator” in the lib_dsp app note. I think there are six instances

Thanks,

Huw

On 15 Jun 2016, at 16:07, Peter Hedinger notifications@github.com<mailto:notifications@github.com> wrote:

Having discussed this with Henk and Ross we have come to the conclusion that at the moment it is still ok to make this major change to the API without adding the deprecated step. There are not enough external users of the library to prevent us from making the change. So, Thomas, go ahead and make the change.

Thanks,

Peter

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/xmos/lib_dsp/issues/52#issuecomment-226706991, or mute the threadhttps://github.com/notifications/unsubscribe/AAqhhnaup2syDLBMksZrMRBoCPLVxzUYks5qMlVugaJpZM4IyIMa.

ThomasGmeinder commented 8 years ago

Resolved by pull request #53