Max/djhackersdev_bemanitools
1
0
Fork 0
Code Issues 72 Pull Requests 15 Actions Packages Projects Releases 20 Wiki Activity

sdvxio-kfca: node detection and allow setting port through envvar - [merged] #116

New Issue
Closed
opened 2019-12-01 20:56:48 +03:00 by icex2 · 24 comments
icex2 commented 2019-12-01 20:56:48 +03:00 (Migrated from github.com)
Copy Link
Copy Source

In GitLab by @xyen on Dec 1, 2019, 18:56

Merges sdvxio-kfca-fixes -> master

Addresses #39

In GitLab by @xyen on Dec 1, 2019, 18:56 _Merges sdvxio-kfca-fixes -> master_ Addresses #39
icex2 commented 2019-12-01 20:59:05 +03:00 (Migrated from github.com)
Copy Link
Copy Source

Can you explain why you read that from an env var? I expected this to be passed along with a configuration.

Can you explain why you read that from an env var? I expected this to be passed along with a configuration.
icex2 commented 2019-12-01 21:00:42 +03:00 (Migrated from github.com)
Copy Link
Copy Source

Would like to avoid implicit defaulting as this might be hiding something from the user configuring things. Better have a default set in a configuration and error if nothing is set here to make it explicit.

Would like to avoid implicit defaulting as this might be hiding something from the user configuring things. Better have a default set in a configuration and error if nothing is set here to make it explicit.
icex2 commented 2019-12-01 21:01:54 +03:00 (Migrated from github.com)
Copy Link
Copy Source

No break when found?

No break when found?
icex2 commented 2019-12-01 21:02:42 +03:00 (Migrated from github.com)
Copy Link
Copy Source

Init successful when no device was found?

Init successful when no device was found?
icex2 commented 2019-12-01 21:03:40 +03:00 (Migrated from github.com)
Copy Link
Copy Source

In GitLab by @xyen on Dec 1, 2019, 19:03

Commented on src/main/sdvxio-kfca/sdvxio.c line 52

would rather not add a dependency to cconfig or parse the command line from here.

In GitLab by @xyen on Dec 1, 2019, 19:03 Commented on [src/main/sdvxio-kfca/sdvxio.c line 52](https://github.com/djhackersdev/bemanitools/compare/723b76219c1c31c9f63c1aaaf208f69eb3a35a4c..5b09ab4aea3dd3db98b5b5f587e160381c177ec0#diff-1fad6a256465ab0642a126f15ff7e64aR52) would rather not add a dependency to cconfig or parse the command line from here.
icex2 commented 2019-12-01 21:04:04 +03:00 (Migrated from github.com)
Copy Link
Copy Source

In GitLab by @xyen on Dec 1, 2019, 19:04

Commented on src/main/sdvxio-kfca/sdvxio.c line 106

Prefer to use last KFCA found if multiple are found.

In GitLab by @xyen on Dec 1, 2019, 19:04 Commented on [src/main/sdvxio-kfca/sdvxio.c line 106](https://github.com/djhackersdev/bemanitools/compare/723b76219c1c31c9f63c1aaaf208f69eb3a35a4c..7e387882dbcdeb2f52cb4773032c150405ba31c1#diff-1fad6a256465ab0642a126f15ff7e64aR106) Prefer to use last KFCA found if multiple are found.
icex2 commented 2019-12-01 21:04:33 +03:00 (Migrated from github.com)
Copy Link
Copy Source

In GitLab by @xyen on Dec 1, 2019, 19:04

Commented on src/main/sdvxio-kfca/sdvxio.c line 117

I don't think we should block on device not being found, it gets logged which should be fine?

In GitLab by @xyen on Dec 1, 2019, 19:04 Commented on [src/main/sdvxio-kfca/sdvxio.c line 117](https://github.com/djhackersdev/bemanitools/compare/723b76219c1c31c9f63c1aaaf208f69eb3a35a4c..7e387882dbcdeb2f52cb4773032c150405ba31c1#diff-1fad6a256465ab0642a126f15ff7e64aR117) I don't think we should block on device not being found, it gets logged which should be fine?
icex2 commented 2019-12-01 21:06:36 +03:00 (Migrated from github.com)
Copy Link
Copy Source

IIRC the other APIs were defined/used that when the device cannot be initialized, e.g. is not found, this functions fails.

IIRC the other APIs were defined/used that when the device cannot be initialized, e.g. is not found, this functions fails.
icex2 commented 2019-12-01 21:07:31 +03:00 (Migrated from github.com)
Copy Link
Copy Source

If multiple can be found, you should log a warning that you used the latest and other devices were skipped as this might be unexpected for some users.

If multiple can be found, you should log a warning that you used the latest and other devices were skipped as this might be unexpected for some users.
icex2 commented 2019-12-01 21:09:36 +03:00 (Migrated from github.com)
Copy Link
Copy Source

I don’t like the idea of introducing another configuration path. I highly suggest making use of the configuration infrastructure somehow. This keeps all key-value props in a single place.

I don’t like the idea of introducing another configuration path. I highly suggest making use of the configuration infrastructure somehow. This keeps all key-value props in a single place.
icex2 commented 2019-12-01 21:32:20 +03:00 (Migrated from github.com)
Copy Link
Copy Source

In GitLab by @xyen on Dec 1, 2019, 19:32

added 1 commit

  • 5b09ab4a - sdvxio-kfca: add some docs and address some comments

Compare with previous version

In GitLab by @xyen on Dec 1, 2019, 19:32 added 1 commit <ul><li>5b09ab4a - sdvxio-kfca: add some docs and address some comments</li></ul> [Compare with previous version](/djhackers/bemanitools/merge_requests/15/diffs?diff_id=1102&start_sha=a5d3c32bedd9d40143dc7cb251e90473cfbffedd)
icex2 commented 2019-12-01 23:01:55 +03:00 (Migrated from github.com)
Copy Link
Copy Source

In GitLab by @xyen on Dec 1, 2019, 21:01

Commented on src/main/sdvxio-kfca/sdvxio.c line 52

changed this line in version 3 of the diff

In GitLab by @xyen on Dec 1, 2019, 21:01 Commented on [src/main/sdvxio-kfca/sdvxio.c line 52](https://github.com/djhackersdev/bemanitools/compare/723b76219c1c31c9f63c1aaaf208f69eb3a35a4c..5b09ab4aea3dd3db98b5b5f587e160381c177ec0#diff-1fad6a256465ab0642a126f15ff7e64aR52) changed this line in [version 3 of the diff](/djhackers/bemanitools/merge_requests/15/diffs?diff_id=1103&start_sha=5b09ab4aea3dd3db98b5b5f587e160381c177ec0#779511ed2fea18b42a3bc0197654e1d9a66abca5_52_55)
icex2 commented 2019-12-01 23:01:55 +03:00 (Migrated from github.com)
Copy Link
Copy Source

In GitLab by @xyen on Dec 1, 2019, 21:01

Commented on src/main/sdvxio-kfca/sdvxio.c line 54

changed this line in version 3 of the diff

In GitLab by @xyen on Dec 1, 2019, 21:01 Commented on [src/main/sdvxio-kfca/sdvxio.c line 54](https://github.com/djhackersdev/bemanitools/compare/723b76219c1c31c9f63c1aaaf208f69eb3a35a4c..5b09ab4aea3dd3db98b5b5f587e160381c177ec0#diff-1fad6a256465ab0642a126f15ff7e64aR54) changed this line in [version 3 of the diff](/djhackers/bemanitools/merge_requests/15/diffs?diff_id=1103&start_sha=5b09ab4aea3dd3db98b5b5f587e160381c177ec0#779511ed2fea18b42a3bc0197654e1d9a66abca5_54_56)
icex2 commented 2019-12-01 23:01:56 +03:00 (Migrated from github.com)
Copy Link
Copy Source

In GitLab by @xyen on Dec 1, 2019, 21:01

added 2 commits

  • 0a5038fd - cconfig: add ability to load configs from default path and specify alternate flag names
  • 7a2d9606 - sdvxio-kfca: use cconfig instead of envvar

Compare with previous version

In GitLab by @xyen on Dec 1, 2019, 21:01 added 2 commits <ul><li>0a5038fd - cconfig: add ability to load configs from default path and specify alternate flag names</li><li>7a2d9606 - sdvxio-kfca: use cconfig instead of envvar</li></ul> [Compare with previous version](/djhackers/bemanitools/merge_requests/15/diffs?diff_id=1103&start_sha=5b09ab4aea3dd3db98b5b5f587e160381c177ec0)
icex2 commented 2019-12-01 23:02:20 +03:00 (Migrated from github.com)
Copy Link
Copy Source

In GitLab by @xyen on Dec 1, 2019, 21:02

resolved all threads

In GitLab by @xyen on Dec 1, 2019, 21:02 resolved all threads
icex2 commented 2019-12-01 23:02:49 +03:00 (Migrated from github.com)
Copy Link
Copy Source

Nit: Documentation

Nit: Documentation
icex2 commented 2019-12-01 23:03:58 +03:00 (Migrated from github.com)
Copy Link
Copy Source

Order of DLLs important?

Order of DLLs important?
icex2 commented 2019-12-01 23:04:49 +03:00 (Migrated from github.com)
Copy Link
Copy Source

👍 for readme

:thumbsup: for readme
icex2 commented 2019-12-01 23:06:56 +03:00 (Migrated from github.com)
Copy Link
Copy Source

In GitLab by @xyen on Dec 1, 2019, 21:06

Commented on doc/sdvxhook/sdvxio-kfca.md line 6

sdvxio isn't a hook dll
there is no ordering

In GitLab by @xyen on Dec 1, 2019, 21:06 Commented on [doc/sdvxhook/sdvxio-kfca.md line 6](https://github.com/djhackersdev/bemanitools/compare/723b76219c1c31c9f63c1aaaf208f69eb3a35a4c..7e387882dbcdeb2f52cb4773032c150405ba31c1#diff-6bea4964ab6f38e4fa064e00597e83d1R6) sdvxio isn't a hook dll there is no ordering
icex2 commented 2019-12-01 23:07:42 +03:00 (Migrated from github.com)
Copy Link
Copy Source

LGTM otherwise.

LGTM otherwise.
icex2 commented 2019-12-01 23:08:34 +03:00 (Migrated from github.com)
Copy Link
Copy Source

right, my bad. Ignore please.

right, my bad. Ignore please.
icex2 commented 2019-12-01 23:14:13 +03:00 (Migrated from github.com)
Copy Link
Copy Source

In GitLab by @xyen on Dec 1, 2019, 21:14

added 1 commit

  • 7e387882 - cconfig: add some documentation to init functions

Compare with previous version

In GitLab by @xyen on Dec 1, 2019, 21:14 added 1 commit <ul><li>7e387882 - cconfig: add some documentation to init functions</li></ul> [Compare with previous version](/djhackers/bemanitools/merge_requests/15/diffs?diff_id=1104&start_sha=7a2d96062931f0f8f3accf27335373db02cab7ca)
icex2 commented 2019-12-01 23:14:20 +03:00 (Migrated from github.com)
Copy Link
Copy Source

In GitLab by @xyen on Dec 1, 2019, 21:14

resolved all threads

In GitLab by @xyen on Dec 1, 2019, 21:14 resolved all threads
icex2 commented 2019-12-01 23:14:38 +03:00 (Migrated from github.com)
Copy Link
Copy Source

In GitLab by @xyen on Dec 1, 2019, 21:14

merged

In GitLab by @xyen on Dec 1, 2019, 21:14 merged
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
acio
bio2
bst
bug
Something isn't working
 ddr
documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 ezusb
feature
gitlab merge request
good first issue
Good for newcomers
 has attachment
help wanted
Extra attention is needed
 high priority
iidx
invalid
This doesn't seem right
 jb
low priority
museca
popn
question
Further information is requested
 sdvx
tech debt
wontfix
This will not be worked on
No labels gitlab merge request
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
Max
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: Max/djhackersdev_bemanitools#116