Hi Jacob On 20 December 2017 at 07:15, Jacob Rosenthal <[email protected]> wrote: >> >> As Vipul mentioned those shells are used only for sensor testing. As >> you probably noticed it will not use sensor API at all but instead it >> uses direct access to the sensor_driver. If you want to use sensors >> via sensor API you should look into sensor_shell from sensor_test > > > I guess I just dont agree that upstreamed code can be considered 'or > testing only' especially when shell is about to be hooked up to coap > access. (which I support) > https://github.com/apache/mynewt-newtmgr/pull/55 >
Well, I'm fine having shell test code for specific sensor driver as it is now. If I want to use given sensor via sensor API I use sensor_test application which gives reference and it seems to support coap as well Regarding PR you mentioned: Well maybe I should not use "shell" in this PR. In fact this is interactive mode for newtmgr coap operations, which I'm not sure will get into upstream. However, glad that somebody is looking into it, comments are welcome. > Upstream code should be reference material and idiomatic. > > Łukasz > On Tue, Dec 19, 2017 at 7:17 AM, Łukasz Rymanowski < > [email protected]> wrote: > >> Hi Jacob >> >> I've look around this code and here is my two cents to the topic :) >> >> On 18 December 2017 at 23:58, Jacob Rosenthal <[email protected]> >> wrote: >> > Im trying to wrap my head around os devices and sensors and am finding >> the >> > current implementations odd. >> > >> > From what I can gather, os_dev_create creates an os device which mainly >> > provides the benefit of being able to open,suspend, resume,close a >> > resource. It does this by calling a callback (device init) function after >> > it finishes whom it expects to register the handler functions >> > >> > But oddly NOTHING in mynewt utilizes suspend or resume functionality, and >> > only adc, uart and pwm even register open/close handlers. >> > >> > Even more odd is that none of the slew of sensors registers ANY handlers >> > with os_device, and are therefore completely wasting the memory >> associated. >> >> Yup, we are missing suspend/resume support as for now. Meaning, >> suspend/resume will not be never called and sadly we are missing even >> API to set suspend/resume callbacks as Vipul mentioned. >> >> However if you build new driver for something what can sleep, it is >> probably good idea to add struct *os_dev to your driver struct and be >> prepared for using it. >> >> > >> > Proof of this lies in the fact that only one out of the six upstream >> sensor >> > implementations (the shell implementations) even bothers to find and open >> > its existing device >> > https://github.com/apache/mynewt-core/blob/master/hw/ >> drivers/sensors/bma253/src/bma253_shell.c >> > >> > The other five leave the os device closed and they make a fresh >> interface. >> > https://github.com/apache/mynewt-core/blob/master/hw/ >> drivers/sensors/bmp280/src/bmp280_shell.c#L39 >> > https://github.com/apache/mynewt-core/blob/master/hw/ >> drivers/sensors/bno055/src/bno055_shell.c#L45 >> > https://github.com/apache/mynewt-core/blob/master/hw/ >> drivers/sensors/bno055/src/bno055_shell.c#L45 >> > https://github.com/apache/mynewt-core/blob/master/hw/ >> drivers/sensors/tcs34725/src/tcs34725_shell.c#L38 >> > https://github.com/apache/mynewt-core/blob/master/hw/ >> drivers/sensors/tsl2561/src/tsl2561_shell.c#L55 >> > >> >> As Vipul mentioned those shells are used only for sensor testing. As >> you probably noticed it will not use sensor API at all but instead it >> uses direct access to the sensor_driver. If you want to use sensors >> via sensor API you should look into sensor_shell from sensor_test >> application >> >> > Which as I said, actually makes sense since opening doesnt do anything >> > >> > Which isnt to tear down any work by anyone involved, Its not like I >> offered >> > any input when this was all being merged originally :) >> > >> > Thoughts on what the usefulness of os_dev is and should be and thus how >> > sensors should or shouldn't be built on top of them? >> >> Sensors now uses os_dev and this is good approach as we want them to >> be aware of power state eventually. There is still lot to do, along >> with full suspend/resume support and patches are always welcome. >> >> hth >> >> Best >> Łukasz >>
