Hi,
thanks for doing the split - my intention was to order the patches in
the series so it can be cut at any point to still produce correct
result. As in fixes first, then new features in order of likelyhood of
being merged. That way - I thought - would make it possible to simply
create some sort of "mergeable" branch up to and including the last
acceptable commit from the PR without causing more work for someone
else. Did not work, hopefully I'll remember to just split things as much
as possible/reasonable from the outset next time. (If not, you can
remind me to do that instead of doing the work yourself.)
As for the comments:
- the coding style errors in avrdx_lowconsole.c and avrdx_init.c are
fixed. Turns out I forgot to checkpatch some files, sorry about that.
Updated branches are avrdx_twi_rfc1-4 and avrdx_twi_rfc1-2. To be sure,
I scanned the series as a whole now and it should hopefully be
everything.
- the TC74Ax driver not being uORB - well, I was not sure whether to
submit the driver at all exactly because it is not done the new way. But
then I noticed another such driver was merged in August so I posted it.
As for the reasons why the driver is not done the new way, there are
two:
First - I did not read the documentation. Over my previous submissions,
I was never able to find any howto for writing drivers. As in "if you
want to make driver for X, do this." (Do not take this as a criticism
please, I was thinking about writing such howto but every time I was
done with a feature, it seemed to me that there is actually nothing to
write about because everything is simple, clear and pretty obvious.) So
I got into the habit of simply looking at other code that does the same
thing and adapting it. Only after the driver was almost done, I found
the documents you linked.
The other reason - and the reason why I did not decide to rewrite the
driver - is that unless I misunderstood the documentation, the new way
uses floats to exchange data and seems more complex overall. My intended
use case is currently 8-bit AVR and I am even not able to compile code
that has floats in it. (That can be probably fixed easily by upgrading
to newer gcc.) Moreover, floats are expensive to handle. So to put it
simple, if I did the driver this way, it would not be of much use to me.
All in all, if the driver gets rejected because it uses deprecated
interface, that would be completely understandable from my point of
view. However, I have to say that I am not planning to rewrite it in any
forseeable future (maybe if it proved easy to have it configurable for
both interfaces - judging from bmp180 driver it could be - but I don't
think I'll find the time even in such case.)
- the enum in the TC74Ax driver - changed to
TC74AX_OPERATION_MODE_STANDBY/OPERATING. As usual, this was based on
another driver. Fixed version is in avrdx_twi_rfc1-3 branch.
Let me know if I missed anything.
On 2025-11-29 08:24, Tomek CEDRO wrote:
Change Request in https://github.com/apache/nuttx/pull/17405:
@linguini1 requested changes on this pull request.
1. I feel bad but should this not be a "uORB" sensor (uORB in quotes
because it's just the new sensor framework)? This is what we're asking
for from contributors going forward and I think it's counterproductive
to keep merging legacy interfaces.
https://nuttx.apache.org/docs/latest/components/drivers/special/sensors.html
https://nuttx.apache.org/docs/latest/components/drivers/special/sensors/sensors_uorb.html
https://nuttx.apache.org/docs/latest/applications/system/uorb/index.html
2. In include/nuttx/sensors/tc74ax.h:
+ * Pre-processor Definitions
+
****************************************************************************/
+
+#define TC74AX_CMD_READ_TEMP 0
+#define TC74AX_CMD_READ_CONFIG 1
+
+#define TC74AX_CONFIG_DATA_READY
+
+/****************************************************************************
+ * Public Types
+
****************************************************************************/
+
+#ifndef __ASSEMBLY__
+
+typedef enum
+{
These should be in CAPS and also maybe a have a prefix unique to the
sensor.
--
CeDeROM, SQ7MHZ, http://www.tomek.cedro.info
On Sat, Nov 29, 2025 at 2:59 AM Tomek CEDRO <[email protected]> wrote:
Thanks Kerogit :-)
Your patches are delivered as following PR:
https://github.com/apache/nuttx/pull/17403
https://github.com/apache/nuttx/pull/17404
https://github.com/apache/nuttx/pull/17405
https://github.com/apache/nuttx/pull/17406
Please follow discussion and review on the github.
For future (maintainers please verify):
* Please do NOT bundle several functional changes into a single
branch.
* Pull Requests are made from branches. These can have separate git
commits but should be focused on a single functional change (i.e. arch
fix, board fix, add sensor driver, update board to use added sensor).
* If we simply push a branch like that to become a Pull Request it
will contain several different functional changes in different areas
and it will be rejected.
* We work on single functional change per PR (and so thus branch).
* You can name your branches like avrdx_twi_rfc1-1, avrdx_twi_rfc1-2,
avrdx_twi_rfc1-3, so we can push them and convert directly into valid
Pull Requests in correct order of dependencies.
I had to split that branch into 4 different branches as described
above to create 4 separate PRs that change different functional parts
of the code. That will help us a lot in review :-)
Take care :-)
Tomek
--
CeDeROM, SQ7MHZ, http://www.tomek.cedro.info
On Fri, Nov 28, 2025 at 3:44 PM <[email protected]> wrote:
>
> Hello,
>
> I would like to submit this patch series for review, comments and
> eventual inclusion - it adds support for Two-Wire interface (I2C and
> SMBus) to AVR DA/DB family of chips. There are also fixes for some bugs.
> Patches are available in a git repository nuttx.git at git.kerogit.eu
> accessible through HTTP/S. (Trying to prevent bot traffic by not posting
> the URL in machine-readable form.) The relevant branch is called
> avrdx_twi_rfc1.
>
> The first part is the I2C driver itself. It only has limited support for
> I2C features - no slave mode, no 10 bit addresses for example (those are
> not supported by hardware directly) but otherwise should be usable.
>
> Second part is a driver for Microchip TC74Ax I2C temperature sensor, it
> was used to develop and test the I2C driver. As for this one, I have a
> question: is there a way of tracking how many users have the device file
> open - or more precisely, how many open descriptors exist? I looked
> around and it seems that some drivers simply do their own counting but
> that feels like something that would fail when the user space forks or
> calls dup or something along those lines.
>
> Third part of the series adds the driver to the example (bread)board -
> breadxavr.
>
> More details about all of these can be found in added documentation and
> also in the Kconfig.
>
> Documentation builds on my system, checkpatch has no complaints. Output
> from test application:
>
> nsh> tc74_test
> tc74_test [4:100]
> nsh> Starting TC74 test
> Temperature read: 23
> Temperature read: 23
>
> As previously, I would like to ask someone with GitHub account to change
> this into a pull request. If I forgot/missed something with regards to
> contributing rules, let me know. Other than that, I'll try to keep an
> eye on GitHub.
>
> Thanks.
