Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dev/admt4000 #2462

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Dev/admt4000 #2462

wants to merge 5 commits into from

Conversation

Lcompo
Copy link

@Lcompo Lcompo commented Feb 19, 2025

Pull Request Description

Please replace this with a detailed description and motivation of the changes.
You can tick the checkboxes below with an 'x' between square brackets or just check them after publishing the PR.
If this PR contains a breaking change, list dependent PRs and try to push all related PRs at the same time.

PR Type

  • Bug fix (change that fixes an issue)
  • New feature (change that adds new functionality)
  • Breaking change (has dependencies in other repos or will cause CI to fail)

PR Checklist

  • I have followed the Coding style guidelines
  • I have performed a self-review of the changes
  • I have commented my code, at least hard-to-understand parts
  • I have build all projects affected by the changes in this PR
  • I have tested in hardware affected projects, at the relevant boards
  • I have signed off all commits from this PR
  • I have updated the documentation (wiki pages, ReadMe etc), if applies

The ADMT4000 is a zero power, multiturn sensor that can count up to 46 turns of an external magnetic field and report the absolute position via SPI.
This adds tinyIIO support for the ADMT4000 device. This exposes output angular measurements in channels.
Add initial project files and IIO example for ADMT4000.
Add README.rst documentation file for project alongside other
documentation related files.
Copy link
Contributor

@amiclaus amiclaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this ready for review? I see that the documentation for the driver is missing

@Lcompo Lcompo marked this pull request as ready for review February 21, 2025 06:30
@cencarna
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Comment on lines +1 to +39
/***************************************************************************//**
* @file admt4000.c
* @brief Implementation of ADMT4000 Driver.
* @author Marc Sosa ([email protected])
* @author Jose Ramon San Buenaventura ([email protected])
********************************************************************************
* Copyright 2024(c) Analog Devices, Inc.
*
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are met:
* - Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* - Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in
* the documentation and/or other materials provided with the
* distribution.
* - Neither the name of Analog Devices, Inc. nor the names of its
* contributors may be used to endorse or promote products derived
* from this software without specific prior written permission.
* - The use of this software may or may not infringe the patent rights
* of one or more patent holders. This license does not release you
* from the requirement that you obtain separate licenses from these
* patent holders to use this software.
* - Use of the software either in source or binary form, must be run
* on or directly connected to an Analog Devices Inc. component.
*
* THIS SOFTWARE IS PROVIDED BY ANALOG DEVICES "AS IS" AND ANY EXPRESS OR
* IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, NON-INFRINGEMENT,
* MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
* IN NO EVENT SHALL ANALOG DEVICES BE LIABLE FOR ANY DIRECT, INDIRECT,
* INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
* LIMITED TO, INTELLECTUAL PROPERTY RIGHTS, PROCUREMENT OF SUBSTITUTE GOODS OR
* SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
* CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
* OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*******************************************************************************/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The No-OS license has changed to BSD 3 clause. Use that instead. Example here: https://github.com/analogdevicesinc/no-OS/blob/main/include/no_os_crc16.h

Comment on lines +41 to +43
/******************************************************************************/
/***************************** Include Files **********************************/
/******************************************************************************/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use these comments for new contributions.

#include "no_os_util.h"
#include "no_os_alloc.h"
#include "no_os_delay.h"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole files uses 4 spaces indentation instead of tabs.

Comment on lines +119 to +120
if (ret < 0)
goto err;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (ret)
        goto err;

goto err;

/** Set Direction **/
ret |= no_os_gpio_direction_output(dev->gpio_coil_rs,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're checking the error code right after the function call, so there is no need to use the | operator.


/* Angle Factor Conversion */
for (i = 0; i < 2; i++)
angle[i] = (float)raw_angle[i] * admt4000_angle_conv_factors[i];
Copy link
Contributor

@CiprianRegus CiprianRegus Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We avoid the use of floating point numbers in the driver layer. Can we use radians instead of degrees here, and then return the angle as milliradians?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants