-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add initial support and doc for ad2s1210 #11
Conversation
71eab14
to
e3f00b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the get* should be get.* methods. For the scale properties, are these writable? How are they used?
All renamed as asked. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need some updates related to the get methods
Updated get methods for Dependent properties. |
+adi/+AD2S1210/Rx.m
Outdated
ExcitationFrequency = 0; | ||
end | ||
|
||
properties (Hidden) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This property is not being used - can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attribute was removed from the driver indeed ; removing it here too to stay in sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I wasn't aware ExcitationFrequency had been removed from the driver. I was actually referring to the FramesCount property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the attribute is still there in the driver? https://github.com/analogdevicesinc/linux/blob/71da83b0b094a92ed4c2b8c21503638b3557ac56/drivers/staging/iio/resolver/ad2s1210.c#L609
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I wasn't aware ExcitationFrequency had been removed from the driver. I was actually referring to the FramesCount property.
SamplesPerFrame property seems to be mandatory; here is the error message I get whent trying to remove it :
Abstract classes cannot be instantiated.
Class 'adi.AD2S1210.Rx' inherits abstract methods or properties but does not implement them.
See the list of methods and properties that 'adi.AD2S1210.Rx' must implement if you do not intend the class to be abstract.
Abstract properties for class adi.AD2S1210.Rx:
SamplesPerFrame % defined in matlabshared.libiio.base
It looks like the attribute is still there in the driver? https://github.com/analogdevicesinc/linux/blob/71da83b0b094a92ed4c2b8c21503638b3557ac56/drivers/staging/iio/resolver/ad2s1210.c#L609
There's actually an upstream proposal to convert it to a channel attribute (thus warranting the need to remove it from the Matlab code for now):
https://lore.kernel.org/all/[email protected]/
@ribdp @tfcollins AFAIK requested changes were implemented so far : any update on this ? |
Similar comment as here: #12 (comment) . The idea is that running that script would update sysobjs.json, which in turn will be used to generate documentation automatically, for the AD4020 and the AD2S1210 |
Thanks for the quick reply : latest commit updates sysobjs.json as per your instructions. |
Confirmed documentation gets generated properly: https://ribdp.github.io/PrecisionToolboxForked/master/sysobjects/adi.AD2S1210.Rx/ Looks good to me. |
0cda0b0
to
ce09788
Compare
@ribdp @tfcollins I rebased and signed all commits with GPG to get them verified, can you please merge this pull request into main branch ?
|
@ribdp @tfcollins There may be a merge issue when signing the commits with GPG according to documentation (https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification#signature-verification-for-rebase-and-merge) :
Do you want me to squash and merge all commits before you push the resulting single commit into the main branch to work around this issue ? |
Hi @apelete I do see another issue with the merge check, although upon looking into it it's not related to this PR's changes. So if you could just resolve the merge conflicts, I'd merge and close both PRs. Squashing might not be necessary now, but if it is I'll do that as well. |
ce09788
to
0231374
Compare
This adds initial support for AD2S1210 Resolver-to-Digital Converter. Signed-off-by: Apelete Seketeli <[email protected]>
Signed-off-by: Apelete Seketeli <[email protected]>
Signed-off-by: Apelete Seketeli <[email protected]>
Signed-off-by: Apelete Seketeli <[email protected]>
Signed-off-by: Apelete Seketeli <[email protected]>
Signed-off-by: Apelete Seketeli <[email protected]>
0231374
to
5bba5da
Compare
@ribdp I rebased on main branch again and solved the conflicts, should be good for merging now. |
This adds initial support and doc for AD2S1210 Resolver-to-Digital Converter.
Matlab implementation currently depends on AD2S1210 Linux driver work-in-progress implementation (see AD2S1210 updates #2261).
Consequently, this is an initial support proposal to build upon based on AD2S1210 Linux driver implementation.