You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/04/08 19:32:21 UTC
[GitHub] [incubator-nuttx] Marco-Krahl opened a new pull request #3487: Adds common master bus interface and implementation for onewire support
Marco-Krahl opened a new pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487
## Summary
This patch series extends 1 wire support by providing a master bus interface that can be used by any driver instance.
This also implements a few small wrapper functions for atomic data transition with the connected device on the bus.
I made a few small changes to the existing internal 1wire interface. Especially the slaves handling is hidden by the master interface and currently not used since it is not helpful.
Optimized handling, especially when only one device is connected on the bus, is determined by the number of maximally allowed slaves, not the number of added slaves to the interface like before.
I think the latter one can lead to a bus error when more than one physical device is connected on the bus, but only one is registered.
I assume maximal slaves should be the correct indicator for that.
## Impact
drivers/1wire and include/nuttx/1wire
## Testing
stnm32f103-minimum (Bluepill)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Marco-Krahl commented on pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
Marco-Krahl commented on pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#issuecomment-816410439
> Very nice @Marco-Krahl did you test it in some board? Should be nice to include a board example for MCUs that support 1-wire
Hi Alan,
yes, I have tested it with the driver for a ds18b20 that I'm currently working on. The board is a bluepill (stm32f103). The driver isn't completed now, but creating an instance by the search algorithm (onewire_search) and reading sensor data is working.
I pushed these changes to ensure that I'm on the right track before finalizing the driver. But once I'm ready I can make the changes here on top, additional a board example.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#issuecomment-819308985
> Mmh ... I think there is something wrong either with Kconfig, make savedefconfig, or with the build system. I disabled the following commands in the new board configuration (sure with make menuconfig) that have been lead to this earlier build error:
> CONFIG_NSH_DISABLE_MB=y
> CONFIG_NSH_DISABLE_MH=y
> CONFIG_NSH_DISABLE_MW=y
>
> I enabled these commands now and the build is going on without any error. Any suggestions for this?
You don't disable them explicitly since these commands are disabled by default with patch:
https://github.com/apache/incubator-nuttx-apps/pull/647
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Marco-Krahl commented on pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
Marco-Krahl commented on pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#issuecomment-816876747
OK fine. Hopefully, I will take progress tomorrow bringing my work a step ongoing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Marco-Krahl commented on a change in pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
Marco-Krahl commented on a change in pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#discussion_r612682876
##########
File path: drivers/1wire/1wire.c
##########
@@ -554,6 +562,7 @@ int onewire_search(FAR struct onewire_master_s *master,
nslaves_match++;
}
+ onewire_sem_post(master);
return (ret < 0) ? ret : nslaves_match;
Review comment:
I will rename err_unlock to unlock because the case in line 565 isn't explicit an error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Marco-Krahl commented on a change in pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
Marco-Krahl commented on a change in pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#discussion_r610730957
##########
File path: include/nuttx/1wire/1wire_master.h
##########
@@ -0,0 +1,179 @@
+/****************************************************************************
+ * include/nuttx/1wire/1wire_master.h
+ *
+ * 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.
+ *
+ ****************************************************************************/
+
+#ifndef __INCLUDE_NUTTX_1WIRE_1WIRE_MASTER_H
+#define __INCLUDE_NUTTX_1WIRE_1WIRE_MASTER_H
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <stdbool.h>
Review comment:
Correct, it should be included since we are using uintx_t types.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Marco-Krahl commented on pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
Marco-Krahl commented on pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#issuecomment-816787659
I will make a new pull request. Merge all patches into one, ok?
I also will revert the mutual exclusive search functionality. The reason for that is a possible deadlock when the search callback tries to read or write on the onewire_master interface. Maybe I will add a wrapper that handles device detection by a passed structure to store the device identifier. This can be safely done in a mutual exclusive state.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Marco-Krahl commented on pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
Marco-Krahl commented on pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#issuecomment-816840687
Hi Alan,
Yes, I'm familiar with git. It's my daily business. But when a quashed single commit is expected git rebase is not the solution. Furthermore, I have no permission to create branches on apache-nuttx but only pull requests from my github account. So I have to make a new pull request, I think, or a forced push that is more or less the same in this context.
What I have to do now?
A pull request with a long-running commit history and adding changes on top? A single squashed commit? What about the new driver I'm working on and the expected board example? Should it be a commit on top or an extra pull request on what?
Personally, I'm not happy with single squashed commits but I'm willing to help out to make the review process as easy as possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Marco-Krahl commented on a change in pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
Marco-Krahl commented on a change in pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#discussion_r610740478
##########
File path: drivers/1wire/1wire.c
##########
@@ -45,6 +45,7 @@
#include <nuttx/kmalloc.h>
#include <nuttx/i2c/i2c_master.h>
+#include <nuttx/1wire/1wire_master.h>
#include <nuttx/drivers/1wire.h>
Review comment:
Yes, you are right. I see also no reason why this is placed in the driver directory.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#discussion_r610611656
##########
File path: include/nuttx/1wire/1wire_master.h
##########
@@ -0,0 +1,179 @@
+/****************************************************************************
+ * include/nuttx/1wire/1wire_master.h
+ *
+ * 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.
+ *
+ ****************************************************************************/
+
+#ifndef __INCLUDE_NUTTX_1WIRE_1WIRE_MASTER_H
+#define __INCLUDE_NUTTX_1WIRE_1WIRE_MASTER_H
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <stdbool.h>
+
+/****************************************************************************
+ * Public Types
+ ****************************************************************************/
+
+struct onewire_dev_s;
+struct onewire_master_s;
+
+struct onewire_config_s
+{
+ uint64_t romcode; /* Unique device identifier */
+};
+
+/****************************************************************************
+ * Public Function Prototypes
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: onewire_search
+ *
+ * Description:
+ * Search all devices from a 1-wire network. This is the 1-wire search
+ * algorithm from Maxim Application Note 187.
+ *
+ * Input Parameters:
+ * master - Pointer to the allocated 1-wire interface
+ * family - Limit search to devices of matching family
+ * alarmonly - Limit search to devices on alarm state
+ * cb_search - Callback to call on each device found
+ * arg - Argument passed to cb_search
+ *
+ * Return Value:
+ * Number of slaves present and matching family.
+ *
+ ****************************************************************************/
+
+int onewire_search(FAR struct onewire_master_s *master,
+ int family,
+ bool alarmonly,
+ CODE void (*cb_search)(int family,
+ uint64_t romcode,
+ FAR void *arg),
+ FAR void *arg);
Review comment:
```suggestion
FAR void *arg);
```
##########
File path: include/nuttx/1wire/1wire_master.h
##########
@@ -0,0 +1,179 @@
+/****************************************************************************
+ * include/nuttx/1wire/1wire_master.h
+ *
+ * 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.
+ *
+ ****************************************************************************/
+
+#ifndef __INCLUDE_NUTTX_1WIRE_1WIRE_MASTER_H
+#define __INCLUDE_NUTTX_1WIRE_1WIRE_MASTER_H
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <stdbool.h>
Review comment:
add #include <stdint.h>
##########
File path: drivers/1wire/ds28e17.c
##########
@@ -45,6 +45,7 @@
#include <nuttx/kmalloc.h>
#include <nuttx/i2c/i2c_master.h>
Review comment:
remove too
##########
File path: drivers/1wire/1wire.c
##########
@@ -45,6 +45,7 @@
#include <nuttx/kmalloc.h>
#include <nuttx/i2c/i2c_master.h>
+#include <nuttx/1wire/1wire_master.h>
#include <nuttx/drivers/1wire.h>
Review comment:
should we move 1wire.h from nuttx/drivers/ to nuttx/1wire too?
##########
File path: include/nuttx/1wire/1wire_master.h
##########
@@ -0,0 +1,179 @@
+/****************************************************************************
+ * include/nuttx/1wire/1wire_master.h
+ *
+ * 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.
+ *
+ ****************************************************************************/
+
+#ifndef __INCLUDE_NUTTX_1WIRE_1WIRE_MASTER_H
+#define __INCLUDE_NUTTX_1WIRE_1WIRE_MASTER_H
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <stdbool.h>
+
+/****************************************************************************
+ * Public Types
+ ****************************************************************************/
+
+struct onewire_dev_s;
+struct onewire_master_s;
+
+struct onewire_config_s
+{
+ uint64_t romcode; /* Unique device identifier */
+};
+
+/****************************************************************************
+ * Public Function Prototypes
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: onewire_search
+ *
+ * Description:
+ * Search all devices from a 1-wire network. This is the 1-wire search
+ * algorithm from Maxim Application Note 187.
+ *
+ * Input Parameters:
+ * master - Pointer to the allocated 1-wire interface
+ * family - Limit search to devices of matching family
+ * alarmonly - Limit search to devices on alarm state
+ * cb_search - Callback to call on each device found
+ * arg - Argument passed to cb_search
+ *
+ * Return Value:
+ * Number of slaves present and matching family.
+ *
+ ****************************************************************************/
+
+int onewire_search(FAR struct onewire_master_s *master,
+ int family,
+ bool alarmonly,
+ CODE void (*cb_search)(int family,
+ uint64_t romcode,
+ FAR void *arg),
+ FAR void *arg);
+
+/****************************************************************************
+ * Name: onewire_write
+ *
+ * Description:
+ * Send a block of data on 1WIRE. Each write operation will be an 'atomic'
+ * operation in the sense that any other 1WIRE actions will be serialized
+ * and pend until this write completes.
+ *
+ * Input Parameters:
+ * master - Device-specific state data
+ * config - Described the 1WIRE configuration
+ * buffer - A pointer to the read-only buffer of data to be written to
+ * device
+ * buflen - The number of bytes to send from the buffer
+ *
+ * Returned Value:
+ * 0: success, <0: A negated errno
+ *
+ ****************************************************************************/
+
+int onewire_write(FAR struct onewire_master_s *master,
+ FAR const struct onewire_config_s *config,
+ FAR const uint8_t *buffer, int buflen);
Review comment:
can we change all buffer pointer type to void * or const void *?
##########
File path: include/nuttx/1wire/1wire_master.h
##########
@@ -0,0 +1,179 @@
+/****************************************************************************
+ * include/nuttx/1wire/1wire_master.h
+ *
+ * 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.
+ *
+ ****************************************************************************/
+
+#ifndef __INCLUDE_NUTTX_1WIRE_1WIRE_MASTER_H
+#define __INCLUDE_NUTTX_1WIRE_1WIRE_MASTER_H
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <stdbool.h>
+
+/****************************************************************************
+ * Public Types
+ ****************************************************************************/
+
+struct onewire_dev_s;
+struct onewire_master_s;
+
+struct onewire_config_s
+{
+ uint64_t romcode; /* Unique device identifier */
+};
+
+/****************************************************************************
+ * Public Function Prototypes
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: onewire_search
+ *
+ * Description:
+ * Search all devices from a 1-wire network. This is the 1-wire search
+ * algorithm from Maxim Application Note 187.
+ *
+ * Input Parameters:
+ * master - Pointer to the allocated 1-wire interface
+ * family - Limit search to devices of matching family
+ * alarmonly - Limit search to devices on alarm state
+ * cb_search - Callback to call on each device found
+ * arg - Argument passed to cb_search
+ *
+ * Return Value:
+ * Number of slaves present and matching family.
+ *
+ ****************************************************************************/
+
+int onewire_search(FAR struct onewire_master_s *master,
+ int family,
+ bool alarmonly,
+ CODE void (*cb_search)(int family,
+ uint64_t romcode,
+ FAR void *arg),
+ FAR void *arg);
+
+/****************************************************************************
+ * Name: onewire_write
+ *
+ * Description:
+ * Send a block of data on 1WIRE. Each write operation will be an 'atomic'
+ * operation in the sense that any other 1WIRE actions will be serialized
+ * and pend until this write completes.
+ *
+ * Input Parameters:
+ * master - Device-specific state data
+ * config - Described the 1WIRE configuration
+ * buffer - A pointer to the read-only buffer of data to be written to
+ * device
+ * buflen - The number of bytes to send from the buffer
+ *
+ * Returned Value:
+ * 0: success, <0: A negated errno
+ *
+ ****************************************************************************/
+
+int onewire_write(FAR struct onewire_master_s *master,
+ FAR const struct onewire_config_s *config,
+ FAR const uint8_t *buffer, int buflen);
+
+/****************************************************************************
+ * Name: onewire_read
+ *
+ * Description:
+ * Receive a block of data from 1WIRE. Each read operation will be an
+ * 'atomic' operation in the sense that any other 1WIRE actions will be
+ * serialized and pend until this read completes.
+ *
+ * Input Parameters:
+ * master - Device-specific state data
+ * config - Described the 1WIRE configuration
+ * buffer - A pointer to a buffer of data to receive the data from the
+ * device
+ * buflen - The requested number of bytes to be read
+ *
+ * Returned Value:
+ * 0: success, <0: A negated errno
+ *
+ ****************************************************************************/
+
+int onewire_read(FAR struct onewire_master_s *master,
+ FAR const struct onewire_config_s *config,
+ FAR uint8_t *buffer, int buflen);
+
+/****************************************************************************
+ * Name: onewire_writeread
+ *
+ * Description:
+ * Receive a block of data from 1WIRE. Each read operation will be an
+ * 'atomic' operation in the sense that any other 1WIRE actions will be
+ * serialized and pend until this read completes.
+ *
+ * Input Parameters:
+ * master - Device-specific state data
+ * config - Described the 1WIRE configuration
+ * wbuffer - A pointer to the read-only buffer of data to be written to
+ * device
+ * wbuflen - The number of bytes to send from the buffer
+ * rbuffer - A pointer to a buffer of data to receive the data from the
+ * device
+ * rbuflen - The requested number of bytes to be read
+ *
+ * Returned Value:
+ * 0: success, <0: A negated errno
+ *
+ ****************************************************************************/
+
+int onewire_writeread(FAR struct onewire_master_s *master,
+ FAR const struct onewire_config_s *config,
+ FAR const uint8_t *wbuffer, int wbuflen,
+ FAR uint8_t *rbuffer, int rbuflen);
+
+/****************************************************************************
+ * Name: onewire_initialize
+ *
+ * Description:
+ * Return 1-wire bus master from 1-wire lower half device.
+ *
+ * Input Parameters:
+ * dev - Pointer to the allocated 1-wire lower half
+ * maxslaves - Maximum number of 1-wire slave devices
+ *
+ ****************************************************************************/
+
+FAR struct onewire_master_s *
+ onewire_initialize(FAR struct onewire_dev_s *dev, int maxslaves);
Review comment:
remove the space at the beginning
##########
File path: drivers/1wire/1wire.c
##########
@@ -45,6 +45,7 @@
#include <nuttx/kmalloc.h>
#include <nuttx/i2c/i2c_master.h>
Review comment:
remove?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Marco-Krahl commented on pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
Marco-Krahl commented on pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#issuecomment-818573135
Note! When sensor polling is disabled for the driver, open the character device in nonblocking mode is required. Otherwise, the sensor will not put out any data.
This is a restriction by the function "fetch" see include/nuttx/sensors/sensor.h
@Alan
I can provide an additional small application to read sensor output data inside the app's repo if you want. Maybe this can speed up your test scenario.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#issuecomment-816822073
> I will make a new pull request. Merge all patches into one, ok?
> I also will revert the mutual exclusive search functionality. The reason for that is a possible deadlock when the search callback tries to read or write on the onewire_master interface. Maybe I will add a wrapper that handles device detection by a passed structure to store the device identifier. This can be safely done in a mutual exclusive state.
You don't need to close and create new PR, you just need to learn the "git rebase -i HEAD~N" command ;-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Marco-Krahl commented on a change in pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
Marco-Krahl commented on a change in pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#discussion_r610751051
##########
File path: drivers/1wire/1wire.c
##########
@@ -45,6 +45,7 @@
#include <nuttx/kmalloc.h>
#include <nuttx/i2c/i2c_master.h>
Review comment:
Only i2c_master.h can be removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#issuecomment-816290881
Very nice @Marco-Krahl did you test it in some board? Should be nice to include a board example for MCUs that support 1-wire
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Marco-Krahl commented on a change in pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
Marco-Krahl commented on a change in pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#discussion_r610729900
##########
File path: include/nuttx/1wire/1wire_master.h
##########
@@ -0,0 +1,179 @@
+/****************************************************************************
+ * include/nuttx/1wire/1wire_master.h
+ *
+ * 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.
+ *
+ ****************************************************************************/
+
+#ifndef __INCLUDE_NUTTX_1WIRE_1WIRE_MASTER_H
+#define __INCLUDE_NUTTX_1WIRE_1WIRE_MASTER_H
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <stdbool.h>
+
+/****************************************************************************
+ * Public Types
+ ****************************************************************************/
+
+struct onewire_dev_s;
+struct onewire_master_s;
+
+struct onewire_config_s
+{
+ uint64_t romcode; /* Unique device identifier */
+};
+
+/****************************************************************************
+ * Public Function Prototypes
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: onewire_search
+ *
+ * Description:
+ * Search all devices from a 1-wire network. This is the 1-wire search
+ * algorithm from Maxim Application Note 187.
+ *
+ * Input Parameters:
+ * master - Pointer to the allocated 1-wire interface
+ * family - Limit search to devices of matching family
+ * alarmonly - Limit search to devices on alarm state
+ * cb_search - Callback to call on each device found
+ * arg - Argument passed to cb_search
+ *
+ * Return Value:
+ * Number of slaves present and matching family.
+ *
+ ****************************************************************************/
+
+int onewire_search(FAR struct onewire_master_s *master,
+ int family,
+ bool alarmonly,
+ CODE void (*cb_search)(int family,
+ uint64_t romcode,
+ FAR void *arg),
+ FAR void *arg);
Review comment:
Oh yes, you are right. Why this was not found by checkpatch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#issuecomment-816871295
> ds18b20
> > Very nice @Marco-Krahl did you test it in some board? Should be nice to include a board example for MCUs that support 1-wire
>
> Hi Alan,
>
> yes, I have tested it with the driver for a ds18b20 that I'm currently working on. The board is a bluepill (stm32f103). The driver isn't completed now, but creating an instance by the search algorithm (onewire_search) and reading sensor data is working.
>
> I pushed these changes to ensure that I'm on the right track before finalizing the driver. But once I'm ready I can make the changes here on top, additional a board example.
Thank is great! I have a ds18b20 here too and wil be happy to test it! Nice work! :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Marco-Krahl commented on a change in pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
Marco-Krahl commented on a change in pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#discussion_r610750872
##########
File path: drivers/1wire/ds28e17.c
##########
@@ -45,6 +45,7 @@
#include <nuttx/kmalloc.h>
#include <nuttx/i2c/i2c_master.h>
Review comment:
No can't be removed. Both headers are necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Marco-Krahl commented on a change in pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
Marco-Krahl commented on a change in pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#discussion_r610738637
##########
File path: drivers/1wire/ds28e17.c
##########
@@ -45,6 +45,7 @@
#include <nuttx/kmalloc.h>
#include <nuttx/i2c/i2c_master.h>
Review comment:
kmalloc include can be remove here but not i2c_master. This existing driver is a 1wire to i2c bridge. So both interfaces are necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Marco-Krahl commented on pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
Marco-Krahl commented on pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#issuecomment-819293346
Mmh ... I think there is something wrong either with Kconfig, make savedefconfig, or with the build system. I disabled the following commands in the new board configuration (sure with make menuconfig) that have been lead to this earlier build error:
CONFIG_NSH_DISABLE_MB=y
CONFIG_NSH_DISABLE_MH=y
CONFIG_NSH_DISABLE_MW=y
I enabled these commands now and the build is going on without any error. Any suggestions for this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Marco-Krahl commented on a change in pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
Marco-Krahl commented on a change in pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#discussion_r610737209
##########
File path: drivers/1wire/1wire.c
##########
@@ -45,6 +45,7 @@
#include <nuttx/kmalloc.h>
#include <nuttx/i2c/i2c_master.h>
Review comment:
That is the existing code not changed by me. I think kmalloc and i2c_master are not used here. There are a few dependencies to i2c (error/info) but this should not come from the i2c_master interface.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#discussion_r612674752
##########
File path: drivers/1wire/1wire.c
##########
@@ -554,6 +562,7 @@ int onewire_search(FAR struct onewire_master_s *master,
nslaves_match++;
}
+ onewire_sem_post(master);
return (ret < 0) ? ret : nslaves_match;
Review comment:
remove line 565 and change line 566 to:
goto err_unlock;
##########
File path: drivers/1wire/1wire.c
##########
@@ -554,6 +562,7 @@ int onewire_search(FAR struct onewire_master_s *master,
nslaves_match++;
}
+ onewire_sem_post(master);
return (ret < 0) ? ret : nslaves_match;
Review comment:
remove line 565 and change line 566 to:
goto err_unlock;
##########
File path: drivers/1wire/1wire.c
##########
@@ -668,7 +677,11 @@ int onewire_search(FAR struct onewire_master_s *master,
*/
}
+ onewire_sem_post(master);
return nslaves_match;
+err_unlock:
+ onewire_sem_post(master);
+ return ret;
Review comment:
change to:
unlock:
onewire_sem_post(master);
return ret < 0 ? ret : nslaves_match;
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#issuecomment-816869553
> Hi Alan,
>
> Yes, I'm familiar with git. It's my daily business. But when a quashed single commit is expected git rebase is not the solution. Furthermore, I have no permission to create branches on apache-nuttx but only pull requests from my github account. So I have to make a new pull request, I think, or a forced push that is more or less the same in this context.
>
> What I have to do now?
> A pull request with a long-running commit history and adding changes on top? A single squashed commit? What about the new driver I'm working on and the expected board example? Should it be a commit on top or an extra pull request on what?
> Personally, I'm not happy with single squashed commits but I'm willing to help out to make the review process as easy as possible.
@Marco-Krahl we can use your branch, squash it and git force, the github will update your PR automatically.
Normally I follow these steps: https://acassis.wordpress.com/2021/03/29/how-i-submit-pull-request-to-nuttx/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Marco-Krahl edited a comment on pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
Marco-Krahl edited a comment on pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#issuecomment-818851859
Damn, this board configuration sucks. I don't know what went wrong in the last build inside the arm-10 container. The detailed view shows nothing about an error.
I have a suspicion. Maybe it can be that the binary exceeds the flash size of 64k since the linker script is not modified. Actually, I don't think so but compilers can produce outcomes of different sizes.
However, without any additional information about what's going wrong here, I'm not able to fix anything.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Marco-Krahl commented on pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
Marco-Krahl commented on pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#issuecomment-818851859
Damn, this board configuration sucks. I don't know what went wrong in the last build inside the arm-10 container. The detailed view shows nothing about an error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Marco-Krahl commented on a change in pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
Marco-Krahl commented on a change in pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#discussion_r610733065
##########
File path: include/nuttx/1wire/1wire_master.h
##########
@@ -0,0 +1,179 @@
+/****************************************************************************
+ * include/nuttx/1wire/1wire_master.h
+ *
+ * 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.
+ *
+ ****************************************************************************/
+
+#ifndef __INCLUDE_NUTTX_1WIRE_1WIRE_MASTER_H
+#define __INCLUDE_NUTTX_1WIRE_1WIRE_MASTER_H
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <stdbool.h>
+
+/****************************************************************************
+ * Public Types
+ ****************************************************************************/
+
+struct onewire_dev_s;
+struct onewire_master_s;
+
+struct onewire_config_s
+{
+ uint64_t romcode; /* Unique device identifier */
+};
+
+/****************************************************************************
+ * Public Function Prototypes
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: onewire_search
+ *
+ * Description:
+ * Search all devices from a 1-wire network. This is the 1-wire search
+ * algorithm from Maxim Application Note 187.
+ *
+ * Input Parameters:
+ * master - Pointer to the allocated 1-wire interface
+ * family - Limit search to devices of matching family
+ * alarmonly - Limit search to devices on alarm state
+ * cb_search - Callback to call on each device found
+ * arg - Argument passed to cb_search
+ *
+ * Return Value:
+ * Number of slaves present and matching family.
+ *
+ ****************************************************************************/
+
+int onewire_search(FAR struct onewire_master_s *master,
+ int family,
+ bool alarmonly,
+ CODE void (*cb_search)(int family,
+ uint64_t romcode,
+ FAR void *arg),
+ FAR void *arg);
+
+/****************************************************************************
+ * Name: onewire_write
+ *
+ * Description:
+ * Send a block of data on 1WIRE. Each write operation will be an 'atomic'
+ * operation in the sense that any other 1WIRE actions will be serialized
+ * and pend until this write completes.
+ *
+ * Input Parameters:
+ * master - Device-specific state data
+ * config - Described the 1WIRE configuration
+ * buffer - A pointer to the read-only buffer of data to be written to
+ * device
+ * buflen - The number of bytes to send from the buffer
+ *
+ * Returned Value:
+ * 0: success, <0: A negated errno
+ *
+ ****************************************************************************/
+
+int onewire_write(FAR struct onewire_master_s *master,
+ FAR const struct onewire_config_s *config,
+ FAR const uint8_t *buffer, int buflen);
Review comment:
Mmh .. this is equal to all other bus interfaces like i2c and spi. So changing it to void* means we should do that also at all the other interfaces. I tried to keep the 1wire master stuff close to i2c master definition.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Marco-Krahl commented on a change in pull request #3487: Adds common master bus interface and implementation for onewire support
Posted by GitBox <gi...@apache.org>.
Marco-Krahl commented on a change in pull request #3487:
URL: https://github.com/apache/incubator-nuttx/pull/3487#discussion_r610734580
##########
File path: include/nuttx/1wire/1wire_master.h
##########
@@ -0,0 +1,179 @@
+/****************************************************************************
+ * include/nuttx/1wire/1wire_master.h
+ *
+ * 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.
+ *
+ ****************************************************************************/
+
+#ifndef __INCLUDE_NUTTX_1WIRE_1WIRE_MASTER_H
+#define __INCLUDE_NUTTX_1WIRE_1WIRE_MASTER_H
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <stdbool.h>
+
+/****************************************************************************
+ * Public Types
+ ****************************************************************************/
+
+struct onewire_dev_s;
+struct onewire_master_s;
+
+struct onewire_config_s
+{
+ uint64_t romcode; /* Unique device identifier */
+};
+
+/****************************************************************************
+ * Public Function Prototypes
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: onewire_search
+ *
+ * Description:
+ * Search all devices from a 1-wire network. This is the 1-wire search
+ * algorithm from Maxim Application Note 187.
+ *
+ * Input Parameters:
+ * master - Pointer to the allocated 1-wire interface
+ * family - Limit search to devices of matching family
+ * alarmonly - Limit search to devices on alarm state
+ * cb_search - Callback to call on each device found
+ * arg - Argument passed to cb_search
+ *
+ * Return Value:
+ * Number of slaves present and matching family.
+ *
+ ****************************************************************************/
+
+int onewire_search(FAR struct onewire_master_s *master,
+ int family,
+ bool alarmonly,
+ CODE void (*cb_search)(int family,
+ uint64_t romcode,
+ FAR void *arg),
+ FAR void *arg);
+
+/****************************************************************************
+ * Name: onewire_write
+ *
+ * Description:
+ * Send a block of data on 1WIRE. Each write operation will be an 'atomic'
+ * operation in the sense that any other 1WIRE actions will be serialized
+ * and pend until this write completes.
+ *
+ * Input Parameters:
+ * master - Device-specific state data
+ * config - Described the 1WIRE configuration
+ * buffer - A pointer to the read-only buffer of data to be written to
+ * device
+ * buflen - The number of bytes to send from the buffer
+ *
+ * Returned Value:
+ * 0: success, <0: A negated errno
+ *
+ ****************************************************************************/
+
+int onewire_write(FAR struct onewire_master_s *master,
+ FAR const struct onewire_config_s *config,
+ FAR const uint8_t *buffer, int buflen);
+
+/****************************************************************************
+ * Name: onewire_read
+ *
+ * Description:
+ * Receive a block of data from 1WIRE. Each read operation will be an
+ * 'atomic' operation in the sense that any other 1WIRE actions will be
+ * serialized and pend until this read completes.
+ *
+ * Input Parameters:
+ * master - Device-specific state data
+ * config - Described the 1WIRE configuration
+ * buffer - A pointer to a buffer of data to receive the data from the
+ * device
+ * buflen - The requested number of bytes to be read
+ *
+ * Returned Value:
+ * 0: success, <0: A negated errno
+ *
+ ****************************************************************************/
+
+int onewire_read(FAR struct onewire_master_s *master,
+ FAR const struct onewire_config_s *config,
+ FAR uint8_t *buffer, int buflen);
+
+/****************************************************************************
+ * Name: onewire_writeread
+ *
+ * Description:
+ * Receive a block of data from 1WIRE. Each read operation will be an
+ * 'atomic' operation in the sense that any other 1WIRE actions will be
+ * serialized and pend until this read completes.
+ *
+ * Input Parameters:
+ * master - Device-specific state data
+ * config - Described the 1WIRE configuration
+ * wbuffer - A pointer to the read-only buffer of data to be written to
+ * device
+ * wbuflen - The number of bytes to send from the buffer
+ * rbuffer - A pointer to a buffer of data to receive the data from the
+ * device
+ * rbuflen - The requested number of bytes to be read
+ *
+ * Returned Value:
+ * 0: success, <0: A negated errno
+ *
+ ****************************************************************************/
+
+int onewire_writeread(FAR struct onewire_master_s *master,
+ FAR const struct onewire_config_s *config,
+ FAR const uint8_t *wbuffer, int wbuflen,
+ FAR uint8_t *rbuffer, int rbuflen);
+
+/****************************************************************************
+ * Name: onewire_initialize
+ *
+ * Description:
+ * Return 1-wire bus master from 1-wire lower half device.
+ *
+ * Input Parameters:
+ * dev - Pointer to the allocated 1-wire lower half
+ * maxslaves - Maximum number of 1-wire slave devices
+ *
+ ****************************************************************************/
+
+FAR struct onewire_master_s *
+ onewire_initialize(FAR struct onewire_dev_s *dev, int maxslaves);
Review comment:
That's new for me and also not caught by checkpatch, but sure I can change it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org