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.

Sign up for GitHub

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

Jump to bottom

Modify the record names to match the LNLS Naming System. #11

Merged
merged 1 commit into from
Sep 20, 2024
Merged

Modify the record names to match the LNLS Naming System. #11

merged 1 commit into from
Sep 20, 2024

Conversation

gustavosr8
Copy link
Contributor

@gustavosr8 gustavosr8 added the enhancement New feature or request label Aug 28, 2024
@gustavosr8 gustavosr8 self-assigned this Aug 28, 2024
@henriquesimoes
Copy link

@anacso17 is the one for reviewing this.

ericonr
ericonr requested changes Aug 28, 2024
View reviewed changes
PVList.md Outdated Show resolved Hide resolved
PVList.md Outdated Show resolved Hide resolved
@gustavosr8 gustavosr8 force-pushed the sirius-std-names branch 3 times, most recently from 0dc6586 to e13dbbc Compare August 30, 2024 19:50
anacso17
anacso17 reviewed Aug 30, 2024
View reviewed changes
PVList.md Outdated Show resolved Hide resolved
anacso17
anacso17 reviewed Aug 30, 2024
View reviewed changes
PVList.md Outdated Show resolved Hide resolved
anacso17
anacso17 reviewed Aug 30, 2024
View reviewed changes
PVList.md Outdated Show resolved Hide resolved
anacso17
anacso17 reviewed Aug 30, 2024
View reviewed changes
PVList.md Outdated Show resolved Hide resolved
anacso17
anacso17 reviewed Aug 30, 2024
View reviewed changes
PVList.md Outdated Show resolved Hide resolved
@anacso17
Copy link

I left some comments in the code. Generally speaking, in the PV naming standard we try to group property names starting with the name of a subdevice if it refers to one, e.g. all PVs for a P2P DCC of a FOFB controller:

IA-01RaBPM:BS-FOFBCtrl:DCCP2PBPMId-SP
IA-01RaBPM:BS-FOFBCtrl:DCCP2PBPMId-RB
IA-01RaBPM:BS-FOFBCtrl:DCCP2PTimeFrameLen-SP
IA-01RaBPM:BS-FOFBCtrl:DCCP2PTimeFrameLen-RB
...

anacso17
anacso17 reviewed Sep 2, 2024
View reviewed changes
iocBoot/iocIpmiMgr/st.cmd Outdated Show resolved Hide resolved
henriquesimoes
henriquesimoes requested changes Sep 2, 2024
View reviewed changes
iocBoot/iocIpmiMgr/st.cmd Outdated Show resolved Hide resolved
PVList.md Outdated Show resolved Hide resolved
PVList.md Show resolved Hide resolved
PVList.md Outdated Show resolved Hide resolved
PVList.md Outdated Show resolved Hide resolved
PVList.md Show resolved Hide resolved
@gustavosr8 gustavosr8 force-pushed the sirius-std-names branch 3 times, most recently from 9cb6657 to a72d38a Compare September 2, 2024 16:51
henriquesimoes
henriquesimoes reviewed Sep 2, 2024
View reviewed changes
PVList.md Outdated Show resolved Hide resolved
PVList.md Outdated Show resolved Hide resolved
iocBoot/iocIpmiMgr/st.cmd Outdated Show resolved Hide resolved
@gustavosr8 gustavosr8 force-pushed the sirius-std-names branch 2 times, most recently from a8febe9 to ef8403f Compare September 6, 2024 17:16
henriquesimoes
henriquesimoes requested changes Sep 9, 2024
View reviewed changes
Copy link

@henriquesimoes henriquesimoes left a comment

Choose a reason for hiding this comment

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

Not strictly related to this PR, but I noticed we are not including rtm_cpu in the substitution tree (I'd expect it to be a direct child from the root). Is that intentional?

ipmiMgrApp/Db/mch_nat.substitutions Outdated Show resolved Hide resolved
ipmiMgrApp/Db/mch_nat.substitutions Outdated Show resolved Hide resolved
PVList.md Outdated Show resolved Hide resolved
ipmiMgrApp/Db/sensor_ai_alias.template Outdated Show resolved Hide resolved
ipmiMgrApp/Db/fru_common_alias.template Outdated Show resolved Hide resolved
@gustavosr8
Copy link
Contributor Author

gustavosr8 commented Sep 10, 2024 •
edited
Loading

Not strictly related to this PR, but I noticed we are not including rtm_cpu in the substitution tree (I'd expect it to be a direct child from the root). Is that intentional?

Yes, we are. The rtm_cpu is an module for MCH and is a child of it:

ipmi-mgr-epics-ioc/PVList.md

Line 104 in ef8403f

### `$(AREA):$(DIS)-RTM_CPU:`

@henriquesimoes
Copy link

The rtm_cpu is an module for MCH

That is what I was missing. Thanks. I thought it was similar to rtm_lamp and rtm_8sfp, which go directly in the root. That makes sense now.

@gustavosr8 gustavosr8 force-pushed the sirius-std-names branch 2 times, most recently from e0d6074 to b184b79 Compare September 10, 2024 16:44
henriquesimoes
henriquesimoes approved these changes Sep 12, 2024
View reviewed changes
Copy link

@henriquesimoes henriquesimoes left a comment

Choose a reason for hiding this comment

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

I reviewed the last changes introduced, and they look right to me. It's good to be merged, IMO.

Nit: typos in the commit message: s/sensors names/sensor names/ and s/choosen/chosen/.

Hint: in order to catch those spelling errors of non-existent words, you can use setlocal spell spelllang=en in your (n)vim buffer when writing the commit message (or any other text). If English is already your default LANG, you may omit the spelllang setting. For other languages, you might also need to install additional dictionaries.

ericonr
ericonr reviewed Sep 12, 2024
View reviewed changes
Copy link
Member

@ericonr ericonr left a comment

Choose a reason for hiding this comment

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

I think the standard for some IOCs has been to simply use P and R as AREA and DIS, respectively. I don't know if it's necessary to move to using the longer names, but I'd at least document this switch in the commit message, if you'd prefer to keep it.

PVList.md Outdated Show resolved Hide resolved
PVList.md Outdated Show resolved Hide resolved
PVList.md Outdated Show resolved Hide resolved
PVList.md Outdated Show resolved Hide resolved
PVList.md Outdated Show resolved Hide resolved
PVList.md Outdated Show resolved Hide resolved
PVList.md Show resolved Hide resolved
@gustavosr8 gustavosr8 force-pushed the sirius-std-names branch 2 times, most recently from 1fbad06 to 7d9169e Compare September 17, 2024 11:22
ericonr
ericonr requested changes Sep 18, 2024
View reviewed changes
iocBoot/iocIpmiMgr/st.cmd Outdated Show resolved Hide resolved
ericonr
ericonr reviewed Sep 20, 2024
View reviewed changes
Copy link
Member

@ericonr ericonr left a comment

Choose a reason for hiding this comment

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

Mention PVList.md on the README so people know what it's about :)

iocBoot/iocIpmiMgr/st.cmd Show resolved Hide resolved
@gustavosr8
Modify the record names to match the LNLS Naming System.
5c74cb1 
The sensor names are chosen based on NAT MCH CLI ones.
Additionally, replaced the generic 'P' and 'R' PV prefixes used in some
IOCs with 'AREA' and 'DIS' to improve consistency and readability.
@gustavosr8 gustavosr8 merged commit a3f6131 into master Sep 20, 2024
1 check passed
@gustavosr8 gustavosr8 deleted the sirius-std-names branch September 20, 2024 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericonr ericonr ericonr approved these changes

@henriquesimoes henriquesimoes henriquesimoes approved these changes

@anacso17 anacso17 Awaiting requested review from anacso17

Assignees

@gustavosr8 gustavosr8

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gustavosr8 @henriquesimoes @anacso17 @ericonr