Consider use of rebase merge of pull requests

nordic-krch commented 4 years ago

Long term it's good to have clean and linear history. Zephyr is using this approach. It's well documented here (https://docs.zephyrproject.org/latest/contribute/index.html#pull-requests-and-issues). I would also recommend taking as much as possible from zephyr work flow like coding guidelines, commit message style, etc. Clean contribution rules will help grow community.

najnesnaj commented 4 years ago

I tested your patch, but much to my frustration (I gave up) : it simply does not work. I most certainly do agree with you that it could be the better solution. But ... it should be working out of the box. (I'm patient, but I rather get my watch working)

I adapted both an existing and installed a new one, following the Readme. Admittedly, I'm not the brightest person in this world, but guess there are many of us ;)

najnesnaj commented 4 years ago

I removed your contribution. Following the Readme step by step gets you nowhere. Environment variables should be unset, the existing .west removed, there should be a script directory under pinetime in order to get west working, there were issues with the boilerplate while compiling a sample (app directory not existant)…

nordic-krch commented 4 years ago

sure, i understand that. I will try to fix it and propose new patch. Few questions:

what platform are you on? linux, win?
have you used west with zephyr before? did you have your own west installation?

I normally use linux virtual machine for zephyr development and there i had west installed so i just checked out pinetime along zephyr repo, modified <work>/.west/config and did west update. I was able to compile pinetime samples then. I also try to checkout everything from scratch on windows and that went on successfully but i did not try to compile it there since had no zephyr sdk installed.

Redferne commented 4 years ago

I really like this idea of adding pinetime as a module, but I too am having issues compiling, with this PR merged.

modified <work>/.west/config

Yeah. How? I did this:

[manifest]
path = zephyr
[zephyr]
base = pinetime

Followed by "west update". It seems to have worked. I could build simple sample: $ west build -p -b pinetime samples/display/st7789v

But building using the out-of-tree drivers is NOT working:

$ west build -p -b pinetime samples/sensor/cst816s
WARNING: ZEPHYR_BASE=/home/x/code/zephyrproject/zephyr in the calling environment will be used, but was set to /home/x/code/zephyrproject/pinetime in west config.
 To disable this warning, execute 'west config --global zephyr.base-prefer env'
-- west build: making build dir /home/x/code/zephyrproject/pinetime/build pristine
-- west build: build configuration:
       source directory: /home/x/code/zephyrproject/pinetime/samples/sensor/cst816s
       build directory: /home/x/code/zephyrproject/pinetime/build
       BOARD: pinetime (origin: command line)
-- west build: generating a build system
Zephyr version: 2.1.99
-- Found PythonInterp: /usr/bin/python3.6 (found suitable version "3.6.9", minimum required is "3.6") 
-- Selected BOARD pinetime
-- Found west: /home/x/.local/bin/west (found suitable version "0.6.3", minimum required is "0.6.0")
-- Loading /home/x/code/zephyrproject/pinetime/boards/arm/pinetime/pinetime.dts as base
Devicetree configuration written to /home/x/code/zephyrproject/pinetime/build/zephyr/include/generated/generated_dts_board.conf
warning: 'title:' in /home/x/code/zephyrproject/pinetime/cmake/../dts/bindings/sensor/hx,hrs3300.yaml is deprecated and will be removed (and was never used). Just put a 'description:' that describes the device instead. Use other bindings as a reference, and note that all bindings were updated recently. Think about what information would be useful to other people (e.g. explanations of acronyms, or datasheet links), and put that in as well. The description text shows up as a comment in the generated header. See yaml-multiline.info for how to deal with multiple lines. You probably want 'description: |'.
Parsing Kconfig tree in /home/x/code/zephyrproject/zephyr/Kconfig
Loaded configuration '/home/x/code/zephyrproject/pinetime/boards/arm/pinetime/pinetime_defconfig'
Merged configuration '/home/x/code/zephyrproject/pinetime/samples/sensor/cst816s/prj.conf'
warning: 'title:' in /home/x/code/zephyrproject/pinetime/cmake/../dts/bindings/sensor/hx,hrs3300.yaml is deprecated and will be removed (and was never used). Just put a 'description:' that describes the device instead. Use other bindings as a reference, and note that all bindings were updated recently. Think about what information would be useful to other people (e.g. explanations of acronyms, or datasheet links), and put that in as well. The description text shows up as a comment in the generated header. See yaml-multiline.info for how to deal with multiple lines. You probably want 'description: |'.

warning: STDOUT_CONSOLE (defined at lib/libc/Kconfig:125) was assigned the value 'y' but got the
value 'n'. You can check symbol information (including dependencies) in the 'menuconfig' interface
(see the Application Development Primer section of the manual), or in the Kconfig reference at
http://docs.zephyrproject.org/latest/reference/kconfig/CONFIG_STDOUT_CONSOLE.html (which is updated
regularly from the master branch). See the 'Setting configuration values' section of the Board
Porting Guide as well.

/home/x/code/zephyrproject/pinetime/samples/sensor/cst816s/prj.conf:5: warning: attempt to assign the value 'y' to the undefined symbol CST816S

/home/x/code/zephyrproject/pinetime/samples/sensor/cst816s/prj.conf:8: warning: attempt to assign the value 'y' to the undefined symbol CST816S_TRIGGER_OWN_THREAD

/home/x/code/zephyrproject/pinetime/samples/sensor/cst816s/prj.conf:9: warning: attempt to assign the value 'y' to the undefined symbol CST816S_TRIGGER

Error: Aborting due to non-whitelisted Kconfig warning
'/home/x/code/zephyrproject/pinetime/samples/sensor/cst816s/prj.conf:5: warning: attempt to
assign the value 'y' to the undefined symbol CST816S'. Note: If this warning doesn't point to an
actual problem, you can add it to the whitelist at the top of
/home/x/code/zephyrproject/zephyr/scripts/kconfig/kconfig.py.

CMake Error at /home/x/code/zephyrproject/zephyr/cmake/kconfig.cmake:217 (message):
  command failed with return code: 1
Call Stack (most recent call first):
  /home/x/code/zephyrproject/zephyr/cmake/app/boilerplate.cmake:461 (include)
  CMakeLists.txt:7 (include)

-- Configuring incomplete, errors occurred!
ERROR: command exited with status 1: /usr/local/bin/cmake -B/home/x/code/zephyrproject/pinetime/build -S/home/x/code/zephyrproject/pinetime/samples/sensor/cst816s -GNinja -DBOARD=pinetime

najnesnaj commented 4 years ago

Krzysztof,

in answer to your questions

-> try to fix it and propose new patch. (Thank you, I think it will render this project very usefull!) -> what platform are you on? linux, win? (linux)

-> have you used west with zephyr before? No, I'm a newby. -> did you have your own west installation? (I tried adapting my existing installation and I tried an installation from scratch)

Suppose you're new to zephyr. You set it up and follow all the steps. (too many if you compare this to arduino) You should be able to compile a sample. (but everything is based on the zephyr directory). If you then set up pinetime, you might have to unset things. Even the fact that a .west is present, might prevent an installation. Even an environment-variable (which is present because you installed zephyr) could prevent a flawless installation.

See also comment above from Redferne.

Kind Regards, Jan

nordic-krch commented 4 years ago

@Redferne

Yeah. How? I did this:

I was not clear then, it should be modified to:

[manifest]
path = pinetime
[zephyr]
base = zephyr

Since path needs to point to where manifest (west.yaml) is located which is pinetime/west.yaml.

nordic-krch commented 4 years ago

@najnesnaj I've updated readme (https://github.com/nordic-krch/pinetime-zephyr/tree/west_ext_application) can you try again? Following sequence should start from scratch:

mkdir work
cd work
west init -m https://github.com/nordic-krch/pinetime-zephyr --mr west_ext_application
west update

after that you should be able to build application using west:

cd zephyr
source zephyr-env.sh
cd ../pinetime/samples/sensor/hrs3300
west build -b pinetime -- -DBOARD_ROOT=../../..

Redferne commented 4 years ago

It worked beatifully! Thanks! I did the old fashion way:

$ mkdir zpt
$ cd zpt
$ west init -m https://github.com/nordic-krch/pinetime-zephyr --mr west_ext_application
$ west update
$ . zephyr/zephyr-env.sh
$ cd pinetime
$ west build -p -b pinetime samples/sensor/hrs3300

However the sample I was interested in did not build properly:

$ west build -p -b pinetime samples/sensor/cst816s
...
/home/x/code/pinetime/zpt/pinetime/samples/sensor/cst816s/src/main.c: In function 'main':
/home/x/code/pinetime/zpt/pinetime/samples/sensor/cst816s/src/main.c:31:42: error: 'DT_INST_0_HYNITRON_CST816S_LABEL' undeclared (first use in this function); did you mean 'DT_INST_0_SITRONIX_ST7789V_LABEL'?
  struct device *dev = device_get_binding(DT_INST_0_HYNITRON_CST816S_LABEL);
                                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                          DT_INST_0_SITRONIX_ST7789V_LABEL
...

nordic-krch commented 4 years ago

it's probably because board dts file do not define this sensor: https://github.com/nordic-krch/pinetime-zephyr/blob/08c73dca6f4d996871b21052a81c28b70143a22e/boards/arm/pinetime/pinetime.dts#L75

Did i copy outdated dts? I took it from https://github.com/najnesnaj/pinetime-zephyr/blob/master/pinetime/pinetime.dts

najnesnaj commented 4 years ago

I tried the "Readme"

step 1 (OK) -> corresponds to step 6 in the getting started zephyr

west build -p -b nrf52840_mdk samples/basic/blinky

step 2 mkdir work (where?) cd work

-> I created under zephyrproject

step3

root@CGE5500-5:~/zephyrproject/work# west init -m https://github.com/najnesnaj/pinetime-zephyr FATAL ERROR: already initialized in /root/zephyrproject, aborting. Note: In your environment, ZEPHYR_BASE is set to: /root/zephyrproject/zephyr

This forces west to search for an installation there.
Try unsetting ZEPHYR_BASE and re-running this command.

OK - > I unset ZEPHYR_BASE

root@CGE5500-5:~/zephyrproject/work# west init -m https://github.com/najnesnaj/pinetime-zephyr FATAL ERROR: already initialized in /root/zephyrproject, aborting.

OK -> I remove .west directory and try again

west init -m https://github.com/najnesnaj/pinetime-zephyr === Initializing in /root/zephyrproject/work --- Cloning manifest repository from https://github.com/najnesnaj/pinetime-zephyr, rev. master Initialized empty Git repository in /root/zephyrproject/work/.west/manifest-tmp/.git/ remote: Enumerating objects: 316, done. remote: Counting objects: 100% (316/316), done. remote: Compressing objects: 100% (170/170), done. remote: Total 2022 (delta 109), reused 249 (delta 84), pack-reused 1706 Receiving objects: 100% (2022/2022), 12.86 MiB | 1.83 MiB/s, done. Resolving deltas: 100% (1153/1153), done. From https://github.com/najnesnaj/pinetime-zephyr

branch master -> FETCH_HEAD
[new branch] master -> origin/master
[new branch] revert-4-west_ext_application -> origin/revert-4-west_ext_application 44ade7c6d31e7371b2a2ec0f6f2f040eed88c0de refs/remotes/origin/master Checking out files: 100% (733/733), done. Branch 'master' set up to track remote branch 'master' from 'origin'. Already on 'master' FATAL ERROR: No "west.yml" in manifest repository (/root/zephyrproject/work/.west/manifest-tmp)

step 4 (new try)

rm work mkdir work root@CGE5500-5:~/zephyrproject/work# west init -m https://github.com/nordic-krch/pinetime-zephyr --mr west_ext_application === Initializing in /root/zephyrproject/work --- Cloning manifest repository from https://github.com/nordic-krch/pinetime-zephyr, rev. west_ext_application Initialized empty Git repository in /root/zephyrproject/work/.west/manifest-tmp/.git/ remote: Enumerating objects: 6, done. remote: Counting objects: 100% (6/6), done. remote: Compressing objects: 100% (6/6), done. remote: Total 1710 (delta 0), reused 4 (delta 0), pack-reused 1704 Receiving objects: 100% (1710/1710), 11.74 MiB | 1.16 MiB/s, done. Resolving deltas: 100% (1043/1043), done. From https://github.com/nordic-krch/pinetime-zephyr

branch west_ext_application -> FETCH_HEAD
[new branch] master -> origin/master
[new branch] west_ext_application -> origin/west_ext_application 08c73dca6f4d996871b21052a81c28b70143a22e refs/remotes/origin/west_ext_application Checking out files: 100% (700/700), done. Branch 'west_ext_application' set up to track remote branch 'west_ext_application' from 'origin'. Switched to a new branch 'west_ext_application' === Initialized. Now run "west update" inside /root/zephyrproject/work.

(OK)

step 5 (OK)

cd ../work/pinetime west build -p -b pinetime samples/basic/blinky

Seems to be working!

Can you create a new "pull merge" request?

najnesnaj commented 4 years ago

@Redferne

I am currently working on the touchscreen driver.

It is most definitely not working.

I split it up in simpler test drivers : -one that reacts on an interrupt -one that react on a presence of the 0x15 address

I will put these online, maybe you can help me out on reading out the touchscreen? https://medium.com/@ly.lee/building-a-rust-driver-for-pinetimes-touch-controller-cbc1a5d5d3e9

Once the basic scripts work I can incorporate them in the zephyr touchscreen driver. Mind you : zephyr cannot handle touchscreens yet, that why I treat it as a sensor...

I can read something, but upon touching it again, it does not deliver.

nordic-krch commented 4 years ago

@najnesnaj maybe we could create issues and assign ourselves to describe who is touching what? I started to look into battery (reporting percent, external power connected and charging).

najnesnaj commented 4 years ago

@nordic-krch I already did some work on the battery : samples/sensor/battery. (it has not been tested) It would be cool if it can be visualized on the screen.

najnesnaj commented 4 years ago

@Redferne I modified the board definition so it includes the touchscreen, the sample cst816s should compile now. But there are issues, I'm kind of stuck .... When touched the touchscreen generates an interrupt. (see samples/basic/testirq) When touched the touchscreen becomes visible on the i2c-bus. (see samples/basic/scani2c) When the touchscreen is visible, one should be able to read 63 consequetive registers (mass read) I had no succes using the standard zephyr function. I tried (see samples/basic/scani2c) msgs[1].buf = &buffer[0]; msgs[1].len = 63U; msgs[1].flags = I2C_MSG_READ | I2C_MSG_STOP; but this executes only once (why?) it should work : see https://medium.com/@ly.lee/building-a-rust-driver-for-pinetimes-touch-controller-cbc1a5d5d3e9

najnesnaj / pinetime-zephyr

Consider use of rebase merge of pull requests #5