Header files shouldn't use "lib_" prefix to match other XMOS libraries
larry-xmos opened this issue · 19 comments
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
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.
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.commailto: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//issues/52#issuecomment-225111097, or mute the threadhttps://github.com/notifications/unsubscribe/AAnPXma5B2U6aJ0Q_qcxc872kq2b2pKrks5qKRBDgaJpZM4IyIMa.
Comments from Thomas -
removing the lib_dsp_ 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_DSP_Library 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
The request was to remove the lib_ prefix, leaving it as dsp_math.h, so there should be no conflict.
OK. I will rename to remove lib_.
I realised that changing all header files from lib_dsp....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 lib_dsp_
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.
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?
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.
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
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.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//issues/52#issuecomment-226206786, or mute the threadhttps://github.com/notifications/unsubscribe/AAmJB_zNWUndQMpXMbtiChxoaW1tN8qSks5qMA3EgaJpZM4IyIMa.
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.commailto: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.commailto: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//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//issues/52#issuecomment-226207952, or mute the threadhttps://github.com/notifications/unsubscribe/AAnPXlY8A9Y-K46o8cx0-ckVnGkJnwbdks5qMA6sgaJpZM4IyIMa.
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.
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.commailto: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//issues/52#issuecomment-226212051, or mute the threadhttps://github.com/notifications/unsubscribe/AAnPXtjEIHBFoCubKM2vUX7-bQSpf_tPks5qMBGQgaJpZM4IyIMa.
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
Done in latest Pull Request #53
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.
Thanks Huw
Fixed 24 instances of “accumulater" and pushed.
Commit Fixed typohttps://github.com//pull/53/commits/3caa5ea9ccfd9fe53219f1d36d54fde53dab7460 was added to the latest pull request #53
Cheers, /T
From: huwg59 <notifications@github.commailto:notifications@github.com>
Reply-To: xmos/lib_dsp <reply@reply.github.commailto:reply@reply.github.com>
Date: Friday, 17 June 2016 10:06
To: xmos/lib_dsp <lib_dsp@noreply.github.commailto:lib_dsp@noreply.github.com>
Cc: thomas <thomas@xmos.commailto:thomas@xmos.com>, Comment <comment@noreply.github.commailto:comment@noreply.github.com>
Subject: Re: [xmos/lib_dsp] 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.commailto: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//issues/52#issuecomment-226706991, or mute the threadhttps://github.com/notifications/unsubscribe/AAqhhnaup2syDLBMksZrMRBoCPLVxzUYks5qMlVugaJpZM4IyIMa.
Resolved by pull request #53