Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exposing ErrorCode API in Tree bindings #2806

Merged
merged 10 commits into from
Mar 21, 2020
Merged

Conversation

imskr
Copy link
Collaborator

@imskr imskr commented Mar 2, 2020

Exposing our DS_ErrorCodeToErrorMessage API to our tree bindings

Closes #2773

@community-tc-integration
Copy link

No Taskcluster jobs started for this pull request
The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

@imskr
Copy link
Collaborator Author

imskr commented Mar 2, 2020

@kdavis-mozilla @reuben Am I doing the right way?

@imskr
Copy link
Collaborator Author

imskr commented Mar 2, 2020

For .NET in nativeimp.cs do i add this for DS_ErrorCodeToErrorMessage?

[DllImport("libdeepspeech.so", CallingConvention = CallingConvention.Cdecl)]
        internal unsafe static extern ErrorCodes DS_ErrorCodeToErrorMessage(int aErrorCode,
                   ref IntPtr** pint);

@imskr imskr marked this pull request as ready for review March 2, 2020 16:07
@carlfm01
Copy link
Collaborator

carlfm01 commented Mar 3, 2020

@imskr
Should be something like :


[DllImport("libdeepspeech.so", CallingConvention = CallingConvention.Cdecl)]
        internal unsafe static extern IntPtr DS_ErrorCodeToErrorMessage(int aErrorCode);

then when you call DS_ErrorCodeToErrorMessage :

return DS_ErrorCodeToErrorMessage(erroCode).IntPtrToString();

@imskr
Copy link
Collaborator Author

imskr commented Mar 3, 2020

@carlfm01 @reuben Please review!

@imskr
Copy link
Collaborator Author

imskr commented Mar 4, 2020

@lissyx Thanks I will make the changes.

@lissyx
Copy link
Collaborator

lissyx commented Mar 4, 2020

@imskr I'm not sure we should do that in this bug or file a followup to address, but once this is live, we might want to take the opportunity to update all docs references of "return zero on success non-zero on failure" to refer to error code structure.

@imskr
Copy link
Collaborator Author

imskr commented Mar 4, 2020

Yes. I think we can make the necessary changes and for docs make another PR maybe.

@imskr
Copy link
Collaborator Author

imskr commented Mar 6, 2020

@lissyx Where should I make changes for the doc? And should I open new PR for the doc?

@lissyx
Copy link
Collaborator

lissyx commented Mar 6, 2020

@lissyx Where should I make changes for the doc? And should I open new PR for the doc?

Which doc changes ?

Those ?:

@imskr I'm not sure we should do that in this bug or file a followup to address, but once this is live, we might want to take the opportunity to update all docs references of "return zero on success non-zero on failure" to refer to error code structure.

@imskr
Copy link
Collaborator Author

imskr commented Mar 6, 2020

@lissyx Where should I make changes for the doc? And should I open new PR for the doc?

Which doc changes ?

Those ?:

@imskr I'm not sure we should do that in this bug or file a followup to address, but once this is live, we might want to take the opportunity to update all docs references of "return zero on success non-zero on failure" to refer to error code structure.

Yes

@imskr
Copy link
Collaborator Author

imskr commented Mar 7, 2020

@carlfm01 Is it good now?

Copy link
Collaborator

@carlfm01 carlfm01 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@imskr
Copy link
Collaborator Author

imskr commented Mar 8, 2020

@lissyx Where should I make changes for the doc? And should I open new PR for the doc?

Which doc changes ?
Those ?:

@imskr I'm not sure we should do that in this bug or file a followup to address, but once this is live, we might want to take the opportunity to update all docs references of "return zero on success non-zero on failure" to refer to error code structure.

Yes

@lissyx Doc updation?

@lissyx
Copy link
Collaborator

lissyx commented Mar 9, 2020

@lissyx Where should I make changes for the doc? And should I open new PR for the doc?

Which doc changes ?
Those ?:

@imskr I'm not sure we should do that in this bug or file a followup to address, but once this is live, we might want to take the opportunity to update all docs references of "return zero on success non-zero on failure" to refer to error code structure.

Yes

@lissyx Doc updation?

Sorry but I still dont know what you are referring to. Please be precise, there are many context-dependant discussions around doc her.

@imskr
Copy link
Collaborator Author

imskr commented Mar 9, 2020

@lissyx sorry for the confusion. You mentioned earlier to update all docs references of "return zero on success non-zero on failure" to refer to error code structure. So this PR is good according to the issue. I was asking should I create new PR for docs reference?

@lissyx
Copy link
Collaborator

lissyx commented Mar 9, 2020

@lissyx sorry for the confusion. You mentioned earlier to update all docs references of "return zero on success non-zero on failure" to refer to error code structure. So this PR is good according to the issue. I was asking should I create new PR for docs reference?

Yes, please.

@imskr
Copy link
Collaborator Author

imskr commented Mar 9, 2020

@lissyx docs where I will make changes are in doc directory right?

@lissyx
Copy link
Collaborator

lissyx commented Mar 10, 2020

@lissyx docs where I will make changes are in doc directory right?

Yeah, but you might want to verify everywhere, just in case.

@lissyx
Copy link
Collaborator

lissyx commented Mar 10, 2020

@lissyx
Copy link
Collaborator

lissyx commented Mar 10, 2020

DeepSpeech.cs(94,82): error CS1503: Argument 1: cannot convert from 'DeepSpeechClient.Enums.ErrorCodes' to 'int' [C:\builds\tc-workdir\DeepSpeech\ds\native_client\dotnet\DeepSpeechClient\DeepSpeechClient.csproj]

@lissyx
Copy link
Collaborator

lissyx commented Mar 10, 2020

There's also a warning:

  DeepSpeech.cs(114,20): warning CS0219: The variable 'exceptionMessage' is assigned but its value is never used [C:\builds\tc-workdir\DeepSpeech\ds\native_client\dotnet\DeepSpeechClient\DeepSpeechClient.csproj]

@imskr
Copy link
Collaborator Author

imskr commented Mar 12, 2020

@lissyx Should I delete line causing warning DeepSpeech.cs(114,20): warning CS0219: The variable 'exceptionMessage' is assigned but its value is never used [C:\builds\tc-workdir\DeepSpeech\ds\native_client\dotnet\DeepSpeechClient\DeepSpeechClient.csproj] ?

I think we can ignore this warning

@lissyx
Copy link
Collaborator

lissyx commented Mar 12, 2020

@lissyx Should I delete line causing warning DeepSpeech.cs(114,20): warning CS0219: The variable 'exceptionMessage' is assigned but its value is never used [C:\builds\tc-workdir\DeepSpeech\ds\native_client\dotnet\DeepSpeechClient\DeepSpeechClient.csproj] ?

I think we can ignore this warning

That'd be my guess, but @carlfm01 is the real source of truth here.

@imskr
Copy link
Collaborator Author

imskr commented Mar 12, 2020

@lissyx For warning DeepSpeech.cs(94,82): error CS1503: Argument 1: cannot convert from 'DeepSpeechClient.Enums.ErrorCodes' to 'int' [C:\builds\tc-workdir\DeepSpeech\ds\native_client\dotnet\DeepSpeechClient\DeepSpeechClient.csproj]

I think we are converting it to NativeImp.DS_ErrorCodeToErrorMessage(resultCode).IntPtrToString()

@imskr
Copy link
Collaborator Author

imskr commented Mar 14, 2020

@carlfm01 How should I get rid of that error?

@carlfm01
Copy link
Collaborator

carlfm01 commented Mar 19, 2020

@lissyx Should I delete line causing warning DeepSpeech.cs(114,20): warning CS0219: The variable 'exceptionMessage' is assigned but its value is never used [C:\builds\tc-workdir\DeepSpeech\ds\native_client\dotnet\DeepSpeechClient\DeepSpeechClient.csproj] ?
I think we can ignore this warning

That'd be my guess, but @carlfm01 is the real source of truth here.

Yes, this is no longer required.

And to convert Enums into ints please use (int)errorCodes. https://stackoverflow.com/questions/943398/get-int-value-from-enum-in-c-sharp

Please debug and make sure that the parsed errorCode is still the desired.

Copy link
Contributor

@reuben reuben left a comment

Choose a reason for hiding this comment

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

Thanks!

@reuben reuben merged commit 5d50d21 into mozilla:master Mar 21, 2020
@lock
Copy link

lock bot commented Apr 25, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Good First Bug] Implement an API to get textual descriptions of error codes
4 participants