[GitHub] mkiiskila commented on a change in pull request #448: [WIP DO NOT MERGE] PWM high level API
mkiiskila commented on a change in pull request #448: [WIP DO NOT MERGE] PWM
high level API
URL: https://github.com/apache/mynewt-core/pull/448#discussion_r131001392
##
File path: apps/pwm_test/src/main.c
##
@@ -0,0 +1,71 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+#include "sysinit/sysinit.h"
+#include
+#include
+#include
+#include
+
+static struct os_dev dev;
+int arg = 0;
+struct pwm_dev *pwm;
+static int value = 1;
+
+int
+main(int argc, char **argv)
+{
+sysinit();
+
+struct nrf52_pwm_chan_cfg chan_conf = {
+.pin = LED_1,
+.inverted = true
+};
+
+os_dev_create(&dev,
+ "pwm",
+ OS_DEV_INIT_KERNEL,
+ OS_DEV_INIT_PRIO_DEFAULT,
+ nrf52_pwm_dev_init,
+ NULL);
+pwm = (struct pwm_dev *) os_dev_open("pwm", 0, NULL);
+
+pwm_chan_config(pwm, 0, &chan_conf);
+pwm_enable_duty_cycle(pwm, 0, 1);
+
+chan_conf.pin = LED_2;
+pwm_chan_config(pwm, 1, &chan_conf);
+pwm_enable_duty_cycle(pwm, 1, value/10);
+
+//changing frequency while playing
Review comment:
Also c++ comments are not allowed according to our coding style. See
CODING_STANDARDS.md
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mkiiskila commented on a change in pull request #448: [WIP DO NOT MERGE] PWM high level API
mkiiskila commented on a change in pull request #448: [WIP DO NOT MERGE] PWM high level API URL: https://github.com/apache/mynewt-core/pull/448#discussion_r131000798 ## File path: apps/pwm_test/pkg.yml ## @@ -0,0 +1,35 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# + +pkg.name: apps/pwm_test +pkg.type: app +pkg.description: Basic example application which blinks an LED. +pkg.author: "Apache Mynewt " +pkg.homepage: "http://mynewt.apache.org/"; +pkg.keywords: + +pkg.deps: +- kernel/os + # - hw/hal + # - hw/cmsis-core +- sys/console/full + # - hw/bsp/nrf52dk +- hw/drivers/pwm + # - "@mynewt_nordic/hw/mcu/nordic_sdk" +- "@mynewt_nordic/hw/drivers/pwm/pwm_nrf52" Review comment: I would remove the dependency on pwm_nrf52, and move it to hw/bsp//pkg.yml instead. Also I would do the os_dev_create() within that BSPs hal_bsp_init(). Make the creation of the device to be a syscfg within the BSP. That would allow you to use this same app with BSPs which use some other MCU (once PWM drivers for those architectures appear :). This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] mkiiskila commented on a change in pull request #448: [WIP DO NOT MERGE] PWM high level API
mkiiskila commented on a change in pull request #448: [WIP DO NOT MERGE] PWM
high level API
URL: https://github.com/apache/mynewt-core/pull/448#discussion_r13089
##
File path: apps/pwm_test/src/main.c
##
@@ -0,0 +1,71 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+#include "sysinit/sysinit.h"
+#include
+#include
+#include
+#include
+
+static struct os_dev dev;
+int arg = 0;
+struct pwm_dev *pwm;
+static int value = 1;
+
+int
+main(int argc, char **argv)
+{
+sysinit();
+
+struct nrf52_pwm_chan_cfg chan_conf = {
Review comment:
Coding style thing; declare variables in the beginning of function, before
other statements. I.e. before call to sysinit().
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mkiiskila commented on a change in pull request #448: [WIP DO NOT MERGE] PWM high level API
mkiiskila commented on a change in pull request #448: [WIP DO NOT MERGE] PWM high level API URL: https://github.com/apache/mynewt-core/pull/448#discussion_r131003957 ## File path: hw/drivers/pwm/src/pwm.c ## @@ -0,0 +1,136 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + + Review comment: I would recommend including here before other #include's. In case there'll be syscfg settings later which affect this file. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] mkiiskila commented on a change in pull request #448: [WIP DO NOT MERGE] PWM high level API
mkiiskila commented on a change in pull request #448: [WIP DO NOT MERGE] PWM high level API URL: https://github.com/apache/mynewt-core/pull/448#discussion_r131003490 ## File path: hw/bsp/nrf52dk/pkg.yml ## @@ -39,8 +39,8 @@ pkg.cflags: - '-DPDM_ENABLED=0' - '-DPERIPHERAL_RESOURCE_SHARING_ENABLED=1' - '-DPWM0_ENABLED=1' -- '-DPWM1_ENABLED=0' -- '-DPWM2_ENABLED=0' +- '-DPWM1_ENABLED=1' +- '-DPWM2_ENABLED=1' Review comment: I wonder if we could make these SDK flags conditional, such that PWMx_ENABLED would be set only if the matching os_dev for that PWM is enabled with that BSP. I.e. in hw/bsp/nrf52dk/pkg.yml, you would have: pkg.cflags.PWM0_ENABLED: -DPWM0_ENABLED and in this file you'd have pkg.cflags: -DPWM0_ENABLED. It *should* work, as newt orders the cflags passed to command line, although it might be a bit too hacky. I.e. depends on newt doing the cflag ordering. Others might have comments regarding this approach? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] mkiiskila commented on a change in pull request #448: [WIP DO NOT MERGE] PWM high level API
mkiiskila commented on a change in pull request #448: [WIP DO NOT MERGE] PWM
high level API
URL: https://github.com/apache/mynewt-core/pull/448#discussion_r131001105
##
File path: apps/pwm_test/src/main.c
##
@@ -0,0 +1,71 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+#include "sysinit/sysinit.h"
+#include
+#include
+#include
+#include
+
+static struct os_dev dev;
+int arg = 0;
+struct pwm_dev *pwm;
+static int value = 1;
+
+int
+main(int argc, char **argv)
+{
+sysinit();
+
+struct nrf52_pwm_chan_cfg chan_conf = {
+.pin = LED_1,
+.inverted = true
+};
+
+os_dev_create(&dev,
Review comment:
I would move this device creation to happen under hw/bsp/, and use
name "pwm0" for the first one.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
