This repository has been archived by the owner. It is now read-only.

Add `devicepkg-dev` which generate the touchscreen udev rule based on the deviceinfo #995

Merged
merged 7 commits into from Dec 14, 2017

Conversation

Projects
None yet
2 participants
@drebrez
Member

drebrez commented Dec 10, 2017

Including devicepkg-dev package in makedepends of the device APKBUILD, allows to call:

  • devicepkg_default_build during the build function which reads the deviceinfo and generate the udev rule for the touchscreen
  • devicepkg_default_package during the package function will install the generated file

It will use the deviceinfo_dev_touchscreen and deviceinfo_dev_touchscreen_calibration variables of the deviceinfo file and creates a rule like this:

SUBSYSTEM=="input", ENV{DEVNAME}=="/dev/input/event2", \
ENV{WL_CALIBRATION}="1.043478 0.000000 -5.217407 0.014562 0.956938 2.798035", \
ENV{ID_INPUT}="1", ENV{ID_INPUT_TOUCHSCREEN}="1"

(PS: for the samsung i9070 I removed the calibration since it's not necessary)

commands used for testing:

sudo rm -f ~/.local/var/pmbootstrap/packages/*/devicepkg-dev-*
sudo rm -f ~/.local/var/pmbootstrap/packages/*/device-samsung-i9070-*
pmbootstrap index
pmbootstrap -y zap

pmbootstrap checksum devicepkg-dev && pmbootstrap build devicepkg-dev --arch armhf
pmbootstrap checksum device-samsung-i9070 && pmbootstrap build device-samsung-i9070 --arch=armhf

pmbootstrap -y zap
pmbootstrap chroot -r -- apk add device-samsung-i9070
pmbootstrap chroot -r -- cat /etc/udev/rules.d/90-touchscreen-dev.rules
@ollieparanoid

Some thoughts:

  • What do you think about making a follow-up PR, that changes existing devices to use devicepkg-dev?
  • I think we should use this by default in the device wizard (with this PR), otherwise people won't use it, or it would make the porting guide more complicated).

Code looks good, I've commented on the error messages. Thank you very much for this contribution @drebrez!

@drebrez

This comment has been minimized.

Show comment
Hide comment
@drebrez

drebrez Dec 11, 2017

Member

@ollieparanoid thanks for the review, yes I also wanted to improve the error messages.
I also agree that we should change all the packages and make it the default also for the wizard.

Member

drebrez commented Dec 11, 2017

@ollieparanoid thanks for the review, yes I also wanted to improve the error messages.
I also agree that we should change all the packages and make it the default also for the wizard.

@drebrez

This comment has been minimized.

Show comment
Hide comment
@drebrez

drebrez Dec 11, 2017

Member

@ollieparanoid did you noticed the mkdir -p "$pkgdir" right before calling devicepkg_default_package in the APKBUILD? this is because the pkg/device-aaa-bbb folder is not present at that point and to install the file from the script we need to know where, so I used "$(find ./pkg -name "device-*")"/etc/udev/rules.d/90-touchscreen-dev.rules,
another solution was to call devicepkg_default_package giving the $pkgdir as an argument, don't know which is better. what you think?

Member

drebrez commented Dec 11, 2017

@ollieparanoid did you noticed the mkdir -p "$pkgdir" right before calling devicepkg_default_package in the APKBUILD? this is because the pkg/device-aaa-bbb folder is not present at that point and to install the file from the script we need to know where, so I used "$(find ./pkg -name "device-*")"/etc/udev/rules.d/90-touchscreen-dev.rules,
another solution was to call devicepkg_default_package giving the $pkgdir as an argument, don't know which is better. what you think?

@ollieparanoid

This comment has been minimized.

Show comment
Hide comment
@ollieparanoid

ollieparanoid Dec 11, 2017

Member

I did not notice it yet, good that you point it out. Passing $pkgdir as argument seems to be less fragile to me, than the find. And we can save one line 👍

Member

ollieparanoid commented Dec 11, 2017

I did not notice it yet, good that you point it out. Passing $pkgdir as argument seems to be less fragile to me, than the find. And we can save one line 👍

@drebrez

This comment has been minimized.

Show comment
Hide comment
@drebrez

drebrez Dec 12, 2017

Member

@ollieparanoid to avoid random problems with the creation of the file depending on the current folder (as explained in chat, with --force it runs in a different location) I've added some arguments to both calls.
I also changed it as the default in the new device wizard.
There is only a problem left, devicepkg_default_build creates the udev rule file only if the deviceinfo_dev_touchscreen variable is specified, but devicepkg_default_package always expect the existence of the generated file. How we proceed there?

extra: is it ok the file name 90-touchscreen-dev.rules? I remember a discussion sometime ago about how to rename that file

Member

drebrez commented Dec 12, 2017

@ollieparanoid to avoid random problems with the creation of the file depending on the current folder (as explained in chat, with --force it runs in a different location) I've added some arguments to both calls.
I also changed it as the default in the new device wizard.
There is only a problem left, devicepkg_default_build creates the udev rule file only if the deviceinfo_dev_touchscreen variable is specified, but devicepkg_default_package always expect the existence of the generated file. How we proceed there?

extra: is it ok the file name 90-touchscreen-dev.rules? I remember a discussion sometime ago about how to rename that file

@ollieparanoid

This comment has been minimized.

Show comment
Hide comment
@ollieparanoid

ollieparanoid Dec 12, 2017

Member

Does always passing $startdir, and constructing the other folders from that, work? See also:

Note: All functions should consider the current working directory as undefined, and should therefore use the abuild-defined directory variables to their advantage.


devicepkg_default_build creates the udev rule file only if the deviceinfo_dev_touchscreen variable is specified, but devicepkg_default_package always expect the existence of the generated file.

Not all devices have a touch screen (e.g. the Ouya). So I think it makes sense to only try to install the file if it was generated before.

What do you think about also installing the deviceinfo file? This one must always exist, so we can use it as check if the script was called inside the APKBUILD. And it makes the package() function shorter, because we don't need to specify it there.

Member

ollieparanoid commented Dec 12, 2017

Does always passing $startdir, and constructing the other folders from that, work? See also:

Note: All functions should consider the current working directory as undefined, and should therefore use the abuild-defined directory variables to their advantage.


devicepkg_default_build creates the udev rule file only if the deviceinfo_dev_touchscreen variable is specified, but devicepkg_default_package always expect the existence of the generated file.

Not all devices have a touch screen (e.g. the Ouya). So I think it makes sense to only try to install the file if it was generated before.

What do you think about also installing the deviceinfo file? This one must always exist, so we can use it as check if the script was called inside the APKBUILD. And it makes the package() function shorter, because we don't need to specify it there.

@drebrez

This comment has been minimized.

Show comment
Hide comment
@drebrez

drebrez Dec 13, 2017

Member

@ollieparanoid as we discussed in chat, here what I've changed:

  • use $startdir and $pkgname arguments for both calls
  • install deviceinfo directly from package script
  • rules file renamed to be generic
  • rules file installed only if present (not needed for some devices)
  • updated default template

I've also added the new deviceinfo_dev_touchscreen_calibration variable in the template, we will also need to update the wiki accordingly.

Member

drebrez commented Dec 13, 2017

@ollieparanoid as we discussed in chat, here what I've changed:

  • use $startdir and $pkgname arguments for both calls
  • install deviceinfo directly from package script
  • rules file renamed to be generic
  • rules file installed only if present (not needed for some devices)
  • updated default template

I've also added the new deviceinfo_dev_touchscreen_calibration variable in the template, we will also need to update the wiki accordingly.

@ollieparanoid

This comment has been minimized.

Show comment
Hide comment
@ollieparanoid

ollieparanoid Dec 14, 2017

Member

Thanks for updating it! I've made some further adjustments:

  • Fixed the testcase (now it does not fail and actually tests if devicepkg-dev builds and its functions run through)
  • Removed the _default_ in devicepkg_build and devicepkg_package. In my opinion, that made it look like it works similar to default_prepare - but in reality it works completely different since we're not calling a function available in the scope of the APKBUILD, but we're executing another script. So this version is shorter and may be a bit less confusing.
  • Wrote a wiki page, and linked to it in the deviceinfo (i9070 and aportgen). That way we can explain what the devicepkg_build function does without requiring the user to dig through the source code.
  • Rebased to master, because one of both Travis runs failed on the qt compatibility check testcase (which is totally unrelated, and the rebase resolved this)

I hope you like the changes, and if you don't feel free to revert them. I've tested that everything is working as expected, before and after these adjustments, so if it's all cool to you we can merge it in my opinion.

Member

ollieparanoid commented Dec 14, 2017

Thanks for updating it! I've made some further adjustments:

  • Fixed the testcase (now it does not fail and actually tests if devicepkg-dev builds and its functions run through)
  • Removed the _default_ in devicepkg_build and devicepkg_package. In my opinion, that made it look like it works similar to default_prepare - but in reality it works completely different since we're not calling a function available in the scope of the APKBUILD, but we're executing another script. So this version is shorter and may be a bit less confusing.
  • Wrote a wiki page, and linked to it in the deviceinfo (i9070 and aportgen). That way we can explain what the devicepkg_build function does without requiring the user to dig through the source code.
  • Rebased to master, because one of both Travis runs failed on the qt compatibility check testcase (which is totally unrelated, and the rebase resolved this)

I hope you like the changes, and if you don't feel free to revert them. I've tested that everything is working as expected, before and after these adjustments, so if it's all cool to you we can merge it in my opinion.

drebrez and others added some commits Dec 10, 2017

devicepkg-dev improvements:
 - use $startdir and $pkgname arguments for both calls
 - install deviceinfo directly from package script
 - rules file renamed to be generic
 - rules file installed only if present (not needed for some devices)
 - updated default template

postmarketOS-Wiki pushed a commit to postmarketOS/wiki that referenced this pull request Dec 14, 2017

@drebrez

This comment has been minimized.

Show comment
Hide comment
@drebrez

drebrez Dec 14, 2017

Member

@ollieparanoid looks good and the changes make sense 👍 . Also thanks for fixing the test case.

Member

drebrez commented Dec 14, 2017

@ollieparanoid looks good and the changes make sense 👍 . Also thanks for fixing the test case.

@ollieparanoid ollieparanoid merged commit 2541505 into master Dec 14, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 64.09%
Details

@ollieparanoid ollieparanoid deleted the feature/devicepkg-generate-udev-rules branch Dec 14, 2017

@ollieparanoid

This comment has been minimized.

Show comment
Hide comment
@ollieparanoid

ollieparanoid Dec 14, 2017

Member

You're welcome!

I've also added the new deviceinfo_dev_touchscreen_calibration variable in the template, we will also need to update the wiki accordingly.

Do you have time for it and do you want to do this?

Member

ollieparanoid commented Dec 14, 2017

You're welcome!

I've also added the new deviceinfo_dev_touchscreen_calibration variable in the template, we will also need to update the wiki accordingly.

Do you have time for it and do you want to do this?

@drebrez

This comment has been minimized.

Show comment
Hide comment
@drebrez

drebrez Dec 14, 2017

Member

@ollieparanoid wiki updated, now we need to update all the existing device packages 😅

Member

drebrez commented Dec 14, 2017

@ollieparanoid wiki updated, now we need to update all the existing device packages 😅

PureTryOut added a commit that referenced this pull request Feb 21, 2018

Add `devicepkg-dev` which generate the touchscreen udev rule based on…
… the deviceinfo (#995)

* Use devicepkg-dev by default in new device wizard
* Add link to reference wiki page
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.