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

Pull Request for extending Network API #618

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

Lucashsmello
Copy link

Related to issue #605.
This pull request adds the following network functionalities:

  • Exit gimx.
  • Get some configuration settings (ex: sensibility)
  • Set some configuration settings
  • Save current calibration/changed settings

See https://github.com/Lucashsmello/GIMX/wiki/Network-api-extended

Notes:

  • When sending values through network, all should be in network byte order.
  • I found strange that in my testing machines, float values should be converted from network byte order. I thought that float values had common representation in all machines. There is a function named float_swap in core/controller.c that do the job.
  • Get/Set configuration settings through network only get/change current profile. Changing profile will be implemented in a future commit. Probably on a new packet type.

@Lucashsmello
Copy link
Author

This failed because a missing change in a file from a submodule. (shared/gimx-network-protocol/protocol.h). I dont know how to make a pull in this case. Should i make pull request for the submodule first?

@Lucashsmello
Copy link
Author

@matlo
Copy link
Owner

matlo commented Apr 9, 2019

Yes, please make a pull request for gimx-network-protocol first. Thanks.

@Lucashsmello Lucashsmello reopened this Apr 9, 2019
core/calibration.c Outdated Show resolved Hide resolved
core/controller.c Outdated Show resolved Hide resolved
core/controller.c Show resolved Hide resolved
core/controller.c Show resolved Hide resolved
core/controller.c Outdated Show resolved Hide resolved
core/controller.c Show resolved Hide resolved
@matlo
Copy link
Owner

matlo commented Apr 9, 2019

As a global remark, why not exposing calibration modes instead of the calibration parameters?

@Lucashsmello
Copy link
Author

Sorry, but what do mean with exposing calibration modes? Exposing pointer to current struct s_mouse_cal? Or creating a function for example void cal_update_parameters(s_mouse_cal* cur_cal)? If so, i do not think is a good idea to expose how configuration settings data is organized which may increase maintenance costs due to the fact that changes in the struct s_mouse_cal may cause more code to be modified.

I forgot to mention that the value INT16_MAX=32767 is used to signal for no modification. This is because the packet is of fixed size so a value must always be sent. Other solutions for not modifying specific calibration values:

  • Client must sent current value. Disadvantage: Client must always store current value.
  • A flag for each calibration value to signal for no modification. Disadvantage: a lot of flags, which increases packet size and decreases readability.
  • Variable sized packet, like s_network_packet_in_report. Disadvantage: more complexity to handle.

@Lucashsmello
Copy link
Author

I just pushed a new commit. I changed .gitmodules to point at my gimx-network-protocol fork so that build checks may pass. Remember to revert it to your own repository url in the future.

@matlo
Copy link
Owner

matlo commented Apr 12, 2019

Regarding my comment on exposing the calibration modes: changing sensitivity is nice for pre-made configs, but changing other settings with no calibration mode is really tedious. You may want to provide network access to the calibration mode, instead of only allowing changing the values.

@Lucashsmello
Copy link
Author

Understood. This is a feature that i may include in a far future. This is not in my priority list.

@Lucashsmello
Copy link
Author

I changed my mind. Thinking about it, it is not hard to implement. It is just not important for my current application. As a bonus, i will expose "exponent factor", "x/y ratio" and profile. I will implement this next sunday.

@Lucashsmello
Copy link
Author

Lucashsmello commented Apr 14, 2019

I exposed exponent and xy_ratio. Also i fixed a bug where "--curses" interface was not being updated when using network api (Lucashsmello@3adfcde). I did not expose changing profile because i have noticed that, looking the source code, selecting mouse and changing profile have a relation that i could not understand. I did not even know that a mouse could be selected.

For selecting calibration mode te and changing calibration test parameters, i am thinking of creating a single packet, that contains 6 bytes:

  • byte 0 is the packet type,
  • byte 1 is the calibration test mode
  • bytes 2-5 compose the calibration parameters value (ex: radius for the circle test)

The problem with this approach is that it assumes all calibration tests to have exactly 1 parameter. But i believe that in the future, if needed, it can be extended to 2 or more parameters with little effort.

A question, does allowing negative values for sensibility makes sense? To invert an axis for example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants